Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix removal of newlines after prettier-ignore #13

Merged
merged 2 commits into from
Oct 13, 2023
Merged

Conversation

tylervz
Copy link
Contributor

@tylervz tylervz commented Oct 12, 2023

Fixes issue #12

The trailing newline is no longer getting removed for the key-value pair after a # prettier-ignore.

For nodes where hasPrettierIgnore is true, the printIgnored method was being called rather than the printer defined by prettier-plugin-properties. Consequently, a hardline was not being added after the key-value pair node was printed.
https://github.com/prettier/prettier/blob/d0b907620b5a59fb75829e1b46e0636a064adae6/src/main/ast-to-doc.js#L101-L102

@eemeli
Copy link
Owner

eemeli commented Oct 12, 2023

Odd that the GitHub CI action didn't trigger; will need to investigate.

Could you drop the dependency updates from this PR, unless they're required for the fix?

@tylervz
Copy link
Contributor Author

tylervz commented Oct 12, 2023

Yes, I'll drop the dependency updates.

I don't have any personal experience with approving GitHub Actions to run on a PR from a public fork, but according to the documentation there ought to be a "Workflow(s) awaiting approval" section on Conversation tab of the PR where you can click "Approve and run".
https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks#about-workflow-runs-from-public-forks

@tylervz
Copy link
Contributor Author

tylervz commented Oct 12, 2023

i think you might need to edit the GitHub Action workflow so it looks like this:

name: Node.js

on:
  pull_request:
    branches: [master]
  pull_request_target:
    types: [opened, synchronize, reopened]
  push:
    branches: [master]
  schedule:
    - cron: '0 12 * * 1' # At noon on Mondays
  workflow_dispatch:

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

After you edit the workflow, I'll probably need to rebase my ignore branch on the updated master.

@eemeli
Copy link
Owner

eemeli commented Oct 13, 2023

It looks like the action had gotten disabled due to a lack of activity, on account of its weekly cron schedule. I'd added that a while back to catch issues from Node.js & other env updates.

@tylervz If you push a commit to the branch (even an empty one) it'll hopefully refresh GitHub's UI so that I can approve a CI run.

Copy link
Owner

@eemeli eemeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a sensible fix. There's some inefficiency that could be avoided by tracking the prettier-ignore state and relying on the order in which print() is called on nodes, but introducing such state is probably only worthwhile if someone complains about the operation being slow.

@eemeli eemeli merged commit 47e7aac into eemeli:master Oct 13, 2023
13 checks passed
@tylervz tylervz deleted the ignore branch October 13, 2023 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants