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

Standardx setup for linting & code formatting #2992

Closed
wants to merge 6 commits into from

Conversation

AWolf81
Copy link
Contributor

@AWolf81 AWolf81 commented May 5, 2019

Description

Similar to closed PR #924 I'd like to add code formatting with pre-commit hook. It will improve development experience as you don't have to care about semicolons, spacings etc. because it will be automatically formatted on save to a consistent code style.

I first wanted to go with prettier but it was problematic to have function paren formatting working.
Then I decided to use standard because it's exactly doing what we need with an exception - it can't be customized.
So in the end I'm using standardx - it is using standard lib under the hood but can be customized.

The only drawback of standardx is that vscode can only use standard and that's why I have added the configuration into package.json as well. So vscode is using package.json and yarn lint is using .eslintrc file.
Not perfect but I think OK to start with. Once the PR is merged / released we can use Standardx in VS code and remove the config from package.json.

Styles we're having (basically according to standard style - just to mention some of the styles):

  • 2 space indent
  • no semis
  • space before parens
  • no-unused vars
  • no-prop warnings JSX (disabled by custom rule)
  • prefer-const

Summary

  • Added vscode settings & recommendations
  • Pre-commit prevents commiting with linting errors (see screenrecording below) (could be by-passed during dev. by --no-verify command line arg)
  • No code changes - I've added some notes & eslint-disable-line to remove some warnings. We could fix these later:
    • The no-useless escapes are disabled inline because in VS code these are not ignored because the ignore setting is not applied - so I've added them
    • Not changed ComponentWillReceiveProps - not sure why the unsafe use warning from linting is not present
  • Some new lint disables because they cause a lot of linting errors & I think it's OK to ignore them:
    • "no-loop-func"
    • "default-case"
    • "no-useless-concat"
    • "jsx-a11y/alt-text" Added because React-App is adding accessibility rules like this.

Points I've noticed during setup

  1. JSX indentation is a bit problematic if not every prop is on it's own line - couldn't get the indentation right if the tag starts with <div someProps={ } and then the next prop in the next line couldn't be indented correctly. (I'll add an inline comment to this later.)
  2. Sometimes an additional semicolon wasn't removed by VS code but it was correctly formatted in command line - I think that's no problem but that was a bit weird.

Issue fixed

Type of changes

  • ⚪ Bug fix (Change that fixed an issue)
  • ⚪ Breaking change (Change that can cause existing functionality to change)
  • 🔘 Improvement (Change that improves the code. Maybe performance or development improvement)
  • ⚪ Feature (Change that adds new functionality)
  • ⚪ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • 🔘 My code follows the project code style
  • ⚪ I have written test for my code and it has been tested
  • ⚪ All existing tests have been passed
  • 🔘 I have attached a screenshot/video to visualize my change if possible

Demo editing with Standardx formatting & linting
Note: The prop typo is not detected in VS code because it's using standard instead of standardx. Formatting is happening on save (ctrl+s).
Screenrecording_Linting_formatting_with_standardx

Demo - Failing pre-commit hook
Note: Coloring of the error is not correctly displayed in the recording. But I've fixed that by forcing the coloring.
Screenrecording_precommit_hook

Here is a test file where I've checked the linting rules.

@@ -1061,23 +1062,21 @@ export default class CodeEditor extends React.Component {
} = this.props
const fontFamily = normalizeEditorFontFamily(this.props.fontFamily)
const width = this.props.width
return (<
div className={
return (<div
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I've meant in the PR comment.

<div className={...}
  ref='root' ...>

Was a problem for standardx formatting. But after adding the new line before className the formating is OK.
I think that's OK but it was a bit weird during setup because the formatter introduced a linting error.

package.json Outdated
"lint-staged": {
"linters": {
"*.{js,jsx,mjs}": [
"npm run lint:fix",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure with this. I think I'd change to lint with-out fix to avoid introducing a problem during committing.

What do you think? Or should we use lint:fix and lint afterwards to check that the fix doesn't introduce an issue.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it is wrong. We should do lint without fixing.

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label May 8, 2019
@ZeroX-DG ZeroX-DG requested a review from Rokt33r May 8, 2019 02:25
@Rokt33r
Copy link
Member

Rokt33r commented May 8, 2019

It sounds quite reasonable to use standardx although it cause tons of conflicts in other prs. @ZeroX-DG How do you think?

@ZeroX-DG
Copy link
Member

ZeroX-DG commented May 8, 2019

@Rokt33r Indeed. Having this can significantly improve the development process. Although it causes conflict in other PR, I think most of the PRs are not very big so contributor can handle with resolving conflicts just fine. If we don't apply this improvement now and wait until all other PRs are merged, during that time, new PRs will jump in and it can cause a huge delay for this PR. I suggest we should apply this improvement now before other big PRs start to jump in.

package.json Outdated
"lint-staged": {
"linters": {
"*.{js,jsx,mjs}": [
"npm run lint:fix",
Copy link
Member

Choose a reason for hiding this comment

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

Yeah it is wrong. We should do lint without fixing.

…g issues (e.g. wrongly formatted code from standardx)
@AWolf81 AWolf81 mentioned this pull request Feb 12, 2020
@AWolf81
Copy link
Contributor Author

AWolf81 commented Feb 15, 2020

We can close this as we're using prettier now (already on master).

@AWolf81 AWolf81 closed this Feb 15, 2020
@AWolf81 AWolf81 deleted the standardx-setup branch February 15, 2020 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review ❇️ Pull request is awaiting a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants