-
Notifications
You must be signed in to change notification settings - Fork 224
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
Added editor config based on styleguide - #1205 #1206
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Should I also add the following? @popojargo and what should be there values
|
As for the max_line_length, we suggest line no longer than 120 but its required in some cases... |
@popojargo I am on it ... so you want max_line_length? |
6c93cf4
to
37b86c5
Compare
@popojargo not sure if I should be force pushing? Are commits squashed on merge and the name of the branch taken as the name of the commit or something to that effect? |
37b86c5
to
bee4730
Compare
This looks cool. What is the value of having this versus setting up prettier that would just run at each commit? |
Having prettier running as a commit hook would be a better solution indeed. |
Also, no need to squash on your branch, you can do it at the merge time. Altough, you should always rebase your branch before merging |
@popojargo @garrensmith I think its wise to have both since why would you code in one format and then have it switch to another format on hook. its better to have some consistency no? Eslint + EditorConfig + Prettier gives you the best of all worlds EditorConfig is also a safety net if you don't npm install and could be applied in a different context also... I pushed prettier using Feel free to play around with it...some attributes I applied for experimentation but there is a discussion on the difference between line length and printWidth. |
72ebd9c
to
24113df
Compare
I figured out how to combine linting with everything else, so I installed However there is still weird behavior with VSCode: prettier/prettier-vscode#371 |
d7491a3
to
820bc73
Compare
Did a little experiment, I also ensured that in the extension, the flag Unrelated, you could use the precommit hook in npm, but Husky gives more power and configuration. |
820bc73
to
66a8775
Compare
@popojargo @garrensmith Done with prettier setup. Let me know what you guys think. The one option I was not sure about was the brace stylings ... some files had spaces in the object literals, others did not... |
} | ||
}, | ||
"lint-staged": { | ||
"*.{js,jsx}": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to lint JSON files or any other files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@popojargo thats up to you guys ... if there is a lot of JSON, sure I can add that in. Let me know. I wonder what @garrensmith @Antonio-Maranhao think as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@iamgollum Can you rebase on master? |
@popojargo because the remote branch already exists - rebase wouldn't work right? |
@popojargo applied merge. Hopefully this is good to go ... if not I can roll back and try again with a different strategy and or git message |
@iamgollum You need to update the package-lock.json as well. Running |
@garrensmith @Antonio-Maranhao Any objections to use prettier? |
80cb985
to
6636903
Compare
6636903
to
7205d6d
Compare
@iamgollum @popojargo This PR seems stale, any chance to revive it? |
Resolves #1205