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

[core] Remove unnecessary plugins in .eslintrc #14161

Merged
merged 2 commits into from Jan 13, 2019

Conversation

WebDeg-Brian
Copy link
Contributor

@WebDeg-Brian WebDeg-Brian commented Jan 12, 2019

Remove unnecessary eslint plugins. I have checked different rules in different files in different locations of the project, and it works fine

@WebDeg-Brian WebDeg-Brian changed the title Remove unnecessary plugins Remove unnecessary plugins in .eslintrc Jan 12, 2019
@oliviertassinari oliviertassinari changed the title Remove unnecessary plugins in .eslintrc [core] Remove unnecessary plugins in .eslintrc Jan 12, 2019
@WebDeg-Brian
Copy link
Contributor Author

@oliviertassinari I just checked the failing test and I don't quite get it.

@joshwooding
Copy link
Member

@oliviertassinari I just checked the failing test and I don't quite get it.

It's because you have branched off of v3.5.1 if you pull in master the tests will pass

@WebDeg-Brian
Copy link
Contributor Author

@joshwooding @oliviertassinari Cheers for the clarification. Is there anything else that I need to do? I'm quite new to contributing to a project

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I went pushing the change further. I have tried to reduce the number of custom rule we have.

@WebDeg-Brian
Copy link
Contributor Author

WebDeg-Brian commented Jan 12, 2019

@oliviertassinari Great, I like the changes. Shall I close this now?

@oliviertassinari
Copy link
Member

@WebDeg-Brian Close? No, we are waiting on the other to review, we will merge once they are happy or after 24h.

@WebDeg-Brian
Copy link
Contributor Author

WebDeg-Brian commented Jan 12, 2019

@oliviertassinari Got it. Btw wouldn't it be better if we use eslint-plugin-prettier as well? Also eslint-config-prettier covers pretty much all the react configurations, so adding them seems redundant to me (correct me if I'm wrong)

@oliviertassinari
Copy link
Member

  • I have already added eslint-config-prettier. It still had to disable some rule.
  • We used to use eslint-plugin-prettier. I like this solution because it was running alongside my eslint IDE. It was removed by @eps1lon. The pros is that it has speed-up our test suite.

@WebDeg-Brian
Copy link
Contributor Author

@oliviertassinari you probably missed my point. I understand that you still have to tweak the rules, but based on what I've seen in your .eslintrc

// Incompatible with prettier
'react/jsx-wrap-multilines': 'off',
'react/jsx-one-expression-per-line': 'off',
'react/jsx-indent': 'off',

And the config's rules:

"use strict";

module.exports = {
  rules: {
    "react/jsx-child-element-spacing": "off",
    "react/jsx-closing-bracket-location": "off",
    "react/jsx-closing-tag-location": "off",
    "react/jsx-curly-spacing": "off",
    "react/jsx-equals-spacing": "off",
    "react/jsx-first-prop-new-line": "off",
    "react/jsx-indent": "off",
    "react/jsx-indent-props": "off",
    "react/jsx-max-props-per-line": "off",
    "react/jsx-one-expression-per-line": "off",
    "react/jsx-props-no-multi-spaces": "off",
    "react/jsx-space-before-closing": "off",
    "react/jsx-tag-spacing": "off",
    "react/jsx-wrap-multilines": "off"
  }
};

I believe we can remove those 3 lines above because they have already been touched by eslint-config-prettier

@eps1lon
Copy link
Member

eps1lon commented Jan 12, 2019

We used to use eslint-plugin-prettier. I like this solution because it was running alongside my eslint IDE. It was removed by @eps1lon. The pros is that it has speed-up our test suite.

I don't see the benefit of eslint-plugin-prettier. With the rise of prettier linting for code style became obsolete. What I would prefer is eslint-config-prettier to make the complete cut. We would remove some noise in our .eslintrc as well as preventing future false positives that are currently not covered.

Rationale for separation of linting for code format:
With the exception of ASI the parser does not care how my code looks. In a dev workflow I don't want to constantly be reminded that I'm missing 2 spaces or I should insert a newline here. It won't have any effect on my code. This only becomes relevant when sharing code so that other people are presented with code that looks similar to their own. Linting however is used to prevent code that will often produce bugs or is in general considered bad practice so it is probably useful to immediately report these issues before I compile my code.

That's why linting and formatting are generally executed at different times when writing code. I personally have gone so far as either having format on save or having a shortcut for formatting that has become second nature after every.

That's the theoretical argument. The practical side was outlined in the original PR #12564. Simply put: eslint can only run on js files while prettier can run on js, json, ts etc. Since we have to execute prettier anyway on the ts files we might as well consolidate code style rules in a single task instead of running some formatting with eslint and some with prettier. If eslint can run on ts files then I would agree that we can move it back there.

@WebDeg-Brian
Copy link
Contributor Author

@eps1lon Fair point.

@oliviertassinari
Copy link
Member

What I would prefer is eslint-config-prettier to make the complete cut. We would remove some noise in our .eslintrc as well as preventing future false positives that are currently not covered.

@eps1lon I agree it's why I have added this package in this pull-request.

I personally have gone so far as either having format on save or having a shortcut for formatting that has become second nature after every.

The one thing I miss is a shorter feedback loop when I have forgotten to run prettier.

@oliviertassinari oliviertassinari merged commit 35fd3b8 into mui:master Jan 13, 2019
@oliviertassinari
Copy link
Member

@WebDeg-Brian Thank you

@WebDeg-Brian
Copy link
Contributor Author

WebDeg-Brian commented Jan 13, 2019

@oliviertassinari Morning. As I said above I believe these 3 lines in the .eslintrc file is redundant.

// Incompatible with prettier
'react/jsx-wrap-multilines': 'off',
'react/jsx-one-expression-per-line': 'off',
'react/jsx-indent': 'off',

Is it possible to remove those?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 13, 2019

Is it possible to remove those?

@WebDeg-Brian I couldn't manage to remove them. If you can, please go ahead :).

@WebDeg-Brian
Copy link
Contributor Author

@oliviertassinari My pleasure. Shall I open a new PR?

@oliviertassinari
Copy link
Member

@WebDeg-Brian Yes.

@eps1lon
Copy link
Member

eps1lon commented Jan 13, 2019

The one thing I miss is a shorter feedback loop when I have forgotten to run prettier.

I understand that. It's not that big of an issue for me anymore since I just autoformat and assume that every line I write isn't formatted anyway. Saves mental overhead if I don't have to think about whether a statement requires a newline, or whitespace or should have a trialing comma etc. I don't think you should fix code style manually.

It may be possible to use yarn prettier as a problem matcher in vscode so that you immediately know if something is formatted. This would help people that need feedback if their code is formatted.

@zannager zannager added the core Infrastructure work going on behind the scenes label Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants