Conversation
|
|
| .boolean("cache").describe("cache", "Enable in memory caching").default("cache", true) | ||
| .boolean("watch").alias("watch", "w").describe("watch", "Watch the filesystem for changes") | ||
| .boolean("watch-stdin").alias("watch-stdin", "stdin").describe("Exit the process when stdin is closed") | ||
| .boolean("help") |
There was a problem hiding this comment.
Honestly, I prefer the old in this scenario. Mainly how the grouping helped readability.
|
//TODO: Update all hashes in stats as it looks like formatted code causes slight different hashes for builds (expected results) |
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
|
Getting there. Will look at this later. |
| const footer = | ||
| self.sourceMapComment.replace( | ||
| /\[url\]/g, | ||
| `data:application/json;charset=utf-8;base64,${new Buffer.alloc((JSON.stringify(sourceMap), "utf8").toString("base64")}` |
|
Could you remove It's doesn't make sense to update the files yet. We can only do that once all other PRs are merged and next is merged in to master. Elsewise it would result in broken PRs/many conflicts. |
|
Yup that's fine. I'll stash away and keep updating branch as we go. |
TheLarkInn
force-pushed
the
formating/prettier
branch
from
5021f5c to
15c037e
Compare
|
Done 👍 |
| indent_style = space | ||
| indent_size = 2 | ||
|
|
||
| [.prettierrc] |
There was a problem hiding this comment.
since they are all the same it could be:
[{*.json,*.yml,.prettierrc}]
indent_style = space
indent_size = 2
There was a problem hiding this comment.
Using .prettierrc.js or .prettierrc.json would be better. Nowadays, many projects supports/requires an extension (prettier, eslint, babel, …)
| "parserOptions": { "ecmaVersion": 2017 }, | ||
| "rules": { | ||
| "quotes": ["error", "double"], | ||
| "prettier/prettier": "warn", |
There was a problem hiding this comment.
More rules can go away because, Prettier is taking care of them
no-extra-semi
semi
brace-style
eol-last
no-empty
no-multiple-empty-lines
no-multi-spaces
no-trailing-spaces
key-spacing
object-curly-spacing
indent
There was a problem hiding this comment.
Better use https://github.com/prettier/eslint-config-prettier#cli-helper-tool, prettier sometimes add something new that can cross with eslint rules
| "mocha": "^3.2.0", | ||
| "mocha-lcov-reporter": "^1.0.0", | ||
| "nsp": "^2.6.1", | ||
| "prettier": "^1.8.2", |
| "lint": "eslint lib bin hot buildin \"test/*.js\" \"test/**/webpack.config.js\" \"test/binCases/**/test.js\" \"examples/**/webpack.config.js\"", | ||
| "lint-files": "npm run lint", | ||
| "lint": "eslint lib bin hot buildin \"test/*.test.js\" \"test/**/webpack.config.js\" \"test/binCases/**/test.js\" \"examples/**/webpack.config.js\"", | ||
| "fix": "npm run lint -- --fix", |
There was a problem hiding this comment.
Since Prettier supports Markdown, JSON, CSS and more, we could add one more rule to fix those as well
"fix:non-js": "prettier --ignore-path .gitignore --write \"**/*.{json,css,md}\"",
and it would be a good idea to add .prettierignore file to exclude the files/paths that we don't want to touch.
|
I would recommend husky + lint-staged to automatically format on commit. Transparent for everyone, automatic. What else? |
|
@MoOx sometimes in prettier have regression(s) in new version and it is broke code or remove some comments, it is not good practice IMHO. |
|
From my experience I never got blocking issue. The same can happen anytimes your format by hand. You can still disable prettier in a file if you got trouble. Anyway, CI should prevent a bad (auto)commited changes. |
|
husky + lint-staged, formats only the files that are changed and not the whole thing so it's pretty safe, and whenever there is a new version and there are bunch of changes it should happen in a separate commit. Here is a bare minimum for ESLint, Prettier, Husky stuff: https://github.com/lipis/prettier-setup/blob/master/package.json |
| @@ -0,0 +1,5 @@ | |||
| { | |||
| "printWidth": 160, | |||
There was a problem hiding this comment.
Why such a high print width? Any more than 100 can lead to too much code printed on one line in some cases.
There was a problem hiding this comment.
I concur. This is a mistake that a lot of people do trying to use prettier for the first time. The line width is not a maximum before you warn like for eslint but how most lines are going to look like. I highly recommend 80 to 100 (maximum).
There was a problem hiding this comment.
100 is plenty for tab based styling. 👍
There was a problem hiding this comment.
160 was chosen to avoid too many changes in the code base due to prettier adoption. This may be decreased later.
|
Just an opinion maybe: we dont use |
|
@cloud-walker by using the plugin you can just do |
| moduleFilenames = ModuleFilenameHelpers.replaceDuplicates(moduleFilenames, (filename, i, n) => { | ||
| for(let j = 0; j < n; j++) | ||
| filename += "*"; | ||
| for (let j = 0; j < n; j++) filename += "*"; |
There was a problem hiding this comment.
for (let j = 0; j < n; filename += "*", j++);
There was a problem hiding this comment.
Couldn't you just return filename + "*".repeat(n); ?
There was a problem hiding this comment.
repeat didn't exist when this code was written... ;)
It doesn't work great for partial staged files/commits. I do that a lot... If you can improve lint-staged in a way that it really modify the staged files and not the working copy that would be great. |
|
Will that make it to v4.0? |
sokra
force-pushed
the
formating/prettier
branch
from
15c037e to
b6396e7
Compare
14 participants
What kind of change does this PR introduce?
Refactor
Did you add tests for your changes?
N/A
If relevant, link to documentation update:
N/A
Summary
Replace beautify with prettier
Does this PR introduce a breaking change?
Shouldn't
Other information
Should be vigilant with tests, inline eslint disable line can break but I think I got them all.