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(prettier): [RO-26549] Added Prettier #82

Closed
wants to merge 1 commit into from

Conversation

otrreeves
Copy link
Contributor

Ticket

https://opentable.atlassian.net/browse/RO-26549

Change Summary

This PR will add Prettier and format all files (using npx prettier --write *, which leverages .editorconfig). It also simplifies the lint configs by removing all rules that are either "controlled" by Prettier or don't need to be overwritten because no violations exist.

NOTE: Lint config files where renamed to specifically include .json because Prettier added trailing commas to them and neither .editorconfig or Prettier have settings to disable this for .*rc files, but Prettier doesn't add trailing commas to .json files because it's not supported for the language. A .prettierignore could be added to ignore them, but then they wouldn't get formatted at all.

@otrreeves otrreeves requested a review from acolchado April 3, 2024 01:32
'controllers/',
'runtime/'
]);
ioc.registerFolders(__dirname, ['controllers/', 'runtime/']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not so much for this one, but in harness, can we please not rewrite the examples in markdown? This is not code so it's optimized for reading in the doc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was formatted by Prettier. .md files could be ignored but then you lose some of the benefits like table formatting, and consistency across .md files in the repo.

I'm not convinced this is an issue. Depending the formatting setup of the target repo that you're copy/pasting into, it would get reformatted anyway right? i.e. If this was left unchanged and you pasted the example code into a repo using Prettier, it would be format like this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a public repo, it's for readability since we don't impose our formatting on anyone.

@@ -152,7 +146,7 @@ To see the latest list of the default dependencies that are injected, check out
List of external dependencies used and exposed by spur-web. They can be found at npmjs.org using their original names.

| Name | Original Module Name |
| :---- | :---- |
| :----------------- | :--------------------------------------------------------------- |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did prettier add these? :---- is a markdown shortcut to make it dynamic and not have to keep track of the length as you write. Can you please revert?

Copy link
Contributor Author

@otrreeves otrreeves Apr 4, 2024

Choose a reason for hiding this comment

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

Prettier handles this, updates with every edit when you save, no need to update manually. Also, the spacing was previously being done manually, but using instead of -

@@ -68,6 +68,7 @@
"eslint": "8.42.0",
"eslint-plugin-node": "11.1.0",
"jest": "29.7.0",
"prettier": "3.2.5",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just realized that this is another dependency we will need to maintain. I also often see warnings from them that require constant version updates.

Should we reconsider?

Copy link
Contributor Author

@otrreeves otrreeves Apr 4, 2024

Choose a reason for hiding this comment

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

It's just a dev dependency so I would think it's easy to maintain especially with renovate doing the heavy-lifting

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's the publishing it, then publishing harness-web steps.
Not heavy lifting but adds a distraction to a set of packages going into maintenance.

@otrreeves otrreeves closed this Apr 4, 2024
@otrreeves otrreeves deleted the chore/ro-26549-add-prettier branch April 4, 2024 02:40
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