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

chore: run prettier with trailing comma all #787

Closed
wants to merge 1 commit into from

Conversation

adamaltman
Copy link
Member

What/Why/How?

I ran npm run prettier before I opened my pull request #786, but I realized I could not commit the prettier changes to that PR because there are so many changes to so many files. I also noticed that we use a lot of trailing commas which were adjusted by prettier.

I adjusted the prettier config to have all for trailing commas. I'm not sure if that is correct or not, but either way, shouldn't we:

  • run prettier and commit, such as I've done in this PR
  • add a GitHub action to run prettier with the check flag: https://prettier.io/docs/en/cli.html#--check to prevent deviations so we don't end up with such a big difference?

Check yourself

  • Code is linted

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

@github-actions
Copy link
Contributor

Coverage report

❌ An unexpected error occurred. For more details, check console

Error: The process '/usr/local/bin/npm' failed with exit code 1
St.
Category Percentage Covered / Total
🟡 Statements 67.87% 2830/4170
🟡 Branches 61% 1561/2559
🔴 Functions 59.19% 451/762
🟡 Lines 67.66% 2624/3878

Test suite run failed

Failed tests: 0/426. Failed suites: 0/75.

Report generated by 🧪jest coverage report action from f617fac

@adamaltman
Copy link
Member Author

Jest: "packages/cli/" coverage threshold for lines (39%) not met: 38.75%

This is purely due to formatting where there are additional lines added from prettier.

@tatomyr
Copy link
Contributor

tatomyr commented Aug 1, 2022

I like the idea. I'll check if there're no substantial conflicts with other active branches and will prepare this for merging.

@tatomyr
Copy link
Contributor

tatomyr commented Aug 1, 2022

I'd like this to have been finished firstly, since it contains a lot of changes. I'll try to do it as soon as possible.

@adamaltman
Copy link
Member Author

@tatomyr also it would be good if someone wrote the github action to run the prettier --check before this would be merged. This already has conflicts and is likely to be easier to produce a new PR at the time -- I only changed the config file (1 line), ran prettier, and git commit. We can redo that after the lint is renamed to styleguide.

@tatomyr tatomyr mentioned this pull request Aug 5, 2022
5 tasks
@tatomyr tatomyr closed this in #797 Aug 8, 2022
@tatomyr tatomyr deleted the chore/prettier-trailing-comma-all branch May 31, 2023 16:45
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.

None yet

3 participants