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

Added editor config based on styleguide - #1205 #1206

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

iamgollum
Copy link
Contributor

@iamgollum iamgollum commented Jul 9, 2019

Resolves #1205

@iamgollum iamgollum changed the title Added base editor config based on styleguide - #1205 Added editor config based on styleguide - #1205 Jul 9, 2019
Copy link
Member

@popojargo popojargo left a comment

Choose a reason for hiding this comment

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

👍

@iamgollum
Copy link
Contributor Author

iamgollum commented Jul 9, 2019

Should I also add the following? @popojargo and what should be there values

tab_width 3?
end_of_line lf? unix?
charset
trim_trailing_whitespace . true?
insert_final_newline . true?
max_line_length . 120? (supported by some editors)

@popojargo
Copy link
Member

  • charset = utf-8
  • trim_trailing_whitespace = true
  • insert_final_new_line = true
  • end_of_line = lf

As for the max_line_length, we suggest line no longer than 120 but its required in some cases...

@iamgollum
Copy link
Contributor Author

iamgollum commented Jul 9, 2019

@popojargo I am on it ... so you want max_line_length?

@iamgollum
Copy link
Contributor Author

iamgollum commented Jul 9, 2019

@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?

@garrensmith
Copy link
Member

This looks cool. What is the value of having this versus setting up prettier that would just run at each commit?

@popojargo
Copy link
Member

Having prettier running as a commit hook would be a better solution indeed.

@popojargo
Copy link
Member

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

@iamgollum
Copy link
Contributor Author

iamgollum commented Jul 13, 2019

@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 husky and pretty-quick. VSCode extension I am using- Prettier - Code formatter however it not aligned with this, because it appears it leverages prettier-eslint which is not supported by pretty-quick: prettier/pretty-quick#22

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.

@iamgollum
Copy link
Contributor Author

iamgollum commented Jul 14, 2019

I figured out how to combine linting with everything else, so I installed lint-staged and applied a regex similar to how you guys did it for the stylecheck script.

However there is still weird behavior with VSCode: prettier/prettier-vscode#371

@iamgollum iamgollum force-pushed the 1205-editorconfig-base branch 6 times, most recently from d7491a3 to 820bc73 Compare July 14, 2019 04:12
@iamgollum
Copy link
Contributor Author

iamgollum commented Jul 14, 2019

Did a little experiment, pretty-quick will only trigger once a file has already been previously formatted in prettier. So first time files - unless applying the VSCode prettier extension - will need to be formatted manually. After that, it will work.

I also ensured that in the extension, the flag Prettier: Eslint Integration in the settings. So after playing around with the it, it appears to work just fine.

Unrelated, you could use the precommit hook in npm, but Husky gives more power and configuration.

@iamgollum
Copy link
Contributor Author

iamgollum commented Jul 14, 2019

@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}": [
Copy link
Member

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?

Copy link
Contributor Author

@iamgollum iamgollum Jul 19, 2019

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

Copy link
Member

@popojargo popojargo left a comment

Choose a reason for hiding this comment

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

👍

@popojargo
Copy link
Member

@iamgollum Can you rebase on master?

@iamgollum
Copy link
Contributor Author

iamgollum commented Sep 18, 2019

@popojargo because the remote branch already exists - rebase wouldn't work right?
I would need to merge in this case...

@iamgollum
Copy link
Contributor Author

iamgollum commented Sep 18, 2019

@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

@popojargo
Copy link
Member

@iamgollum You need to update the package-lock.json as well. Running npm i should solve that

@popojargo
Copy link
Member

@garrensmith @Antonio-Maranhao Any objections to use prettier?

@wohali wohali changed the base branch from master to main October 30, 2020 19:52
@wohali
Copy link
Member

wohali commented Oct 30, 2020

@iamgollum @popojargo This PR seems stale, any chance to revive it?

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.

Consider adding an .editorconfig to enforce aspects of the style guide
4 participants