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

Add eslint-prettier #242

Merged
merged 10 commits into from
Sep 23, 2018
Merged

Add eslint-prettier #242

merged 10 commits into from
Sep 23, 2018

Conversation

LukeSheard
Copy link
Contributor

@LukeSheard LukeSheard commented Sep 18, 2018

With similar intentions to #224 this adds eslint-config-prettierand eslint-plugin-prettier to the eslint config, and adds a pre-commit hook which automatically formats all the javascript files for you.

  • 4a534b7 adds the configuration to check for style errors
  • 33592ba adds the pre-commit hook
  • babb867 is an autoformat of the codebase to make sure this PR passes CI.

@texodus
Copy link
Member

texodus commented Sep 19, 2018

This is great, thanks for the PR!

As Perspective is a multi-language project (C++, Javascript and soon Python, plus non-languages like HTML and LESS), I believe it is important to impose at least some basic consistency across the codebase proper where applicable. We'll make a similar request to #224 when I have time to review the clang-format options in more detail, but for now I believe these overrides to prettier defaults are justified:

  • bracketSpacing: false IMO the default is not consistent with the rest of the project - it is e.g. not the default for clang-format braced initializers, which they justify under Cpp11BracedListStyle here.
  • tabWidth: 4 Again, simply for consistency with Added clang-format file #224 and the extent LESS, HTML, JSON, etc formatting.
  • Worth also mentioning that we should probably have consistent printWidth, as I (and I assume any other developer) will not be switching IDEs when implementing changes across several Perspective languages. I do tend to agree however that Added clang-format file #224's printWidth is a bit on the conservative side, so this is probably fine at 200 for now.

Personally, I am disinterested in the details of code formatting outside of consistency - assuming you concur, would you mind amending this commit by adding the above 2 overrides to the prettier configuration in 4a534b7, and redoing babb867 to respect these changes?

@LukeSheard
Copy link
Contributor Author

LukeSheard commented Sep 19, 2018

Very reasonable request. I was also unsure about printWidth being so large, but people are often strongly opinionated about these things so I just set it to something that was agreeable to sort out the other inconsistencies in the code. We can always update later.

I'll update the options now and re-run babb867.

@texodus texodus merged commit 85bf8c9 into finos:master Sep 23, 2018
@LukeSheard LukeSheard deleted the eslint-prettier branch September 23, 2018 15:51
@texodus texodus mentioned this pull request Sep 24, 2018
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.

2 participants