Uh oh!
There was an error while loading. Please reload this page.
[WIP] Prettier plugin for eslint - #703
Conversation
dsych
commented
Apr 9, 2017
As stated before, outdated dependency is the reason why the build fails. |
humphd
commented
Apr 9, 2017
Filed adobe#13290 upstream to see if we can land this there vs. here. If they don't want it, we can move forward with doing it just in our fork. |
dsych
commented
Apr 10, 2017
@humphd I was thinking about submitting PR to grunt-eslint with the request to update their dependencies. |
humphd
commented
Apr 10, 2017
@dsych go for it. |
dsych
commented
Apr 10, 2017
@humphd I fixed the problem with incompatible dependencies, just needed to update to the latest version of grunt-eslint which uses the latest version of eslint. Now it is pretty much ready, |
humphd
left a comment
There was a problem hiding this comment.
This is very cool.
One thing I noticed running it is that we are including the following submodule in our src list:
modified: src/extensions/default/brackets-paste-and-indent (modified content)
We shouldn't be touching that file.
I also tried running this on the code, and put up a compare branch to show the difference, see mozilla:master...humphd:prettier-plugin. From looking at this, we should fix the following:
- Use 4-space indents vs. 2-space
- Fix the way
/** ... */comments get done, they look odd
We should add some kind of precommit hook, see https://github.com/prettier/prettier#pre-commit-hook-for-changed-files
I'll also share this compare branch with Adobe and see what they think. It would be ideal to do this upstream vs. here, if possible.
| "start": "http-server -p 8000 --cors" | ||
| }, | ||
| "license": "MIT" | ||
| "name": "bramble", |
humphd
commented
Apr 13, 2017
Prettier 1.0 release today http://jlongster.com/prettier-1.0. We should watch for updates on the plugin dep. |
Finally, I am aware that there are a bunch of "test" commits in there, will rebase later. |
| 'exec:clean-nls', | ||
| 'usemin' | ||
| 'usemin', | ||
| 'githooks' |
There was a problem hiding this comment.
This seems wrong to me. We'd want this to run automatically, not just when you manually run build. That is, when I commit, or maybe even when I just stage changes, it should run. There should be no manual intervention required, or people will forget/not-know to do it.
There was a problem hiding this comment.
@humphd This is required to create githook scripts. The formatting does run automatically on before every commit. My comment above
Before building, I suggest running "grunt eslint:src-fix" because now we don't have --fix option in default eslint task.
Only refers to the first run of the build. Eslint does not have auto-fixing set, so styling will fail the build and you will never create githook scripts. So instead you need to format code manually once, and then it will run properly on every commit. After formatted code is committed to the master, there will be no need for manual intervention.
dsych
commented
Apr 19, 2017
@humphd At this point, it does not seem that Adobe is really interested in formatting with prettier, so should we just keep on working on it here or give them more time to think? |
humphd
commented
Apr 19, 2017
We should probably just do it here. Can you fix the merge conflict you have? |
dsych
commented
Apr 19, 2017
@humphd fixed the merge conflict, however now Travic is failing the build because of the prettier rules in the eslint task. I really don't to commit all of the formatted files now, because it will make the PR messy. |
humphd
commented
Apr 20, 2017
OK, great. I'm not going to try and land this while the other students are still scrambling to get PRs done this week. I'll circle back on this later. Good work. |
humphd
commented
Apr 23, 2017
Question about the git-hooks: will what you did work on Windows? |
Tested with GitBash for Windows and CMD for Windows 10 before the Anniversary update. Everything runs smoothly. |
dsych
commented
May 17, 2017
@humphd Are you still interested in this PR or should I close it? |
humphd
commented
May 17, 2017
No, I'm still interested. We just froze the tree for a launch next week, but we'll come back to this. Software takes time, don't give up! |
dsych
commented
May 30, 2017
@humphd Congrats on a new release, can we continue now? |
humphd
commented
Jun 6, 2017
Two things I want to figure out here:
|
Pomax
commented
Jun 6, 2017
@humph what is the issue with comments? Is this a thing that we need to fix "in prettier" or is this a style thing we'd have to correct for in all our files? |
@humphd what is weird about comments, in your opinion? Here is a link to the sample from the formatted branch you did earlier. I believe we can overwrite comment formatting from prettier in favor of eslint. Check out guidance and decide if this is an acceptable way of formatting. I believe that eslint formats comment by default based on this issue For the PRs that do not require formatting, are we talking about specific paths that should be excluded or a user prompt to decide if we want to format? |
Adding code auto formatting with Prettier through grunt-eslint. See discussion. In this version eslint-config-prettier and eslint-plugin-prettier for eslint are utilized in conjunction with grunt-eslint to auto-format code in watched directories.
While I got everything it working, there is a problem with grunt-eslint dependencies. It depends on the 3.0.0 version of Eslint (see), however, eslint-plugin-prettier uses a minimum of 3.14.0 of Eslint. If not satisfied, the following problem arises. So I had to manually, update dependencies for grunt-eslint, which is not the best way in my opinion. Apart form that, everything is operational.
Moreover, I have another version of achieving the same result of prettier formatting bypassing additional plugins. It does the direct calls to prettier through grunt-exec. Although I only got it formatting the whole src folder, it also works. I have a couple of ideas of how to restrict formatting, but they are not working properly. Let me know if you are interested in that approach more.
cc @humphd