Formatting #913
Formatting #913
Conversation
.prettierrc
Outdated
| "printWidth": 120, | ||
| "singleQuote": true, | ||
| "semi": false, | ||
| "trailingComma": "all" |
There was a problem hiding this comment.
this is the only nit I have, I'm personally not a fan of the trailing comma though I will defer to the team's majority vote. @floating what's your stance?
There was a problem hiding this comment.
Can we also add "jsxSingleQuote": true?
There was a problem hiding this comment.
Removing trailing commas, it's often a contentious one.
There was a problem hiding this comment.
Not really a fan of single quote in JSX tbh, however I can see both sides of the argument - "mirror HTML preferences" vs "JSX is not HTML". I think the double quotes help to visually distinguish between JSX string literals, string literals elsewhere and template literals in JSX. Or maybe it's just what I'm used to - React docs feature double-quoted JSX and it's the default in ESLint. Everywhere else, there's the expected endless debate:
airbnb/javascript#629
airbnb/javascript#269 (comment)
xojs/eslint-config-xo#78
prettier/prettier#1080
I'm not precious about it anyway, it's probably more of an aesthetic decision than anything, and any kind of codified formatting is better than none.
There was a problem hiding this comment.
Yeah, I think single quote consistency is better and is already the style the codebase.
There was a problem hiding this comment.
Added jsxSingleQuote and updated. We'll probably need to ditch all the non-config changes and recreate the PR when it is time to merge anyway; the formatting changes in this PR are just to show what the formatted code would look like with these settings.
|
one more thing is we should wait until we rebase |
|
I think we might actually want to wait until the first develop/canary release goes to production before making these types of changes. Also, please wait for my review on this one as well. |
| @@ -76,7 +76,7 @@ describe('token metadata lookup', () => { | |||
| expect(tokenDecimalsInput.value).toEqual('18') | |||
| expect(tokenChainSelect.textContent).toEqual('Mainnet') | |||
| }) | |||
| }, 1000) | |||
| }, 2000) | |||
There was a problem hiding this comment.
are these tests regularly taking longer than 1 second? seems like something we should address as no individual unit test should be taking anywhere near this long
There was a problem hiding this comment.
also please try not to pile a bunch of unrelated changes into one PR, better to make them separate PRs so we can track changes more clearly going forward
There was a problem hiding this comment.
We'll probably need to ditch all the non-config changes and recreate the PR when it is time to merge anyway; the formatting changes in this PR are just to show what the formatted code would look like with these settings.
There was a problem hiding this comment.
When the tests run on every push, you know pretty quickly if they are flaky. I should have forced husky pre-push not to run rather than commit more changes. However, as mentioned above I am not expecting this will be merged, better to take the config changes and reapply formatting to create a new PR when we are ready.
There was a problem hiding this comment.
Marked as draft to make the above more obvious
mholtzman
added
the
WIP
|
Closing in favour of #1113 |
Using prettier, lint-staged and husky to remove formatting nits from all PRs going forward. This does not include linting, saving that for another PR.
Prettier options: https://prettier.io/docs/en/options.html
Changes
npm run formatandnpm run format:checkPrettier settings
printWidth: 120 (better display on larger monitors)
singleQuote: true (single quotes used for all string literals apart from JSX)
jsxSingleQuote: true (single quotes used for all JSX string literals)
semi: false (no semicolons)
In-editor formatting
Prettier VSCode formatter - not required for development but I usually have this configured to format on save:
https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode