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

chore(package): Add ESLint #1796

Closed
wants to merge 14 commits into from
Closed

chore(package): Add ESLint #1796

wants to merge 14 commits into from

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Mar 7, 2019

This adds a basic ESLint configuration to the project which doesn’t require changing almost every file like #1483 (the goal is to slowly enable rules across multiple PRs).

Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Nice work!

Apart from the comments, I also have a more general question:

Quite a number of times multiple variables are declared like this:

var foo,
    bar;

These were aligned using 4 spaces, so that bar is aligned under foo. The new indentation rules forbid this style.
So should try to change the ESLint config or should we just use a few vars more?

.eslintrc Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
gulpfile.js Show resolved Hide resolved
@ExE-Boss ExE-Boss force-pushed the eslint branch 3 times, most recently from f6b4a1b to c56a848 Compare March 7, 2019 23:22
.eslintrc Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
@RunDevelopment RunDevelopment mentioned this pull request Mar 8, 2019
code.js Outdated Show resolved Hide resolved
examples.js Outdated Show resolved Hide resolved
examples.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
gulpfile.js Outdated Show resolved Hide resolved
@LeaVerou
Copy link
Member

LeaVerou commented Mar 8, 2019

As I have expressed in the other issue, I disagree with any linting being classed as "error". It should be warnings, and errors should be reserved for actual JS errors. A wrong indent is not the same importance as a JS parse error that breaks the whole script.

@mAAdhaTTah
Copy link
Member

@LeaVerou Is that to suggest we can merge in PRs with warnings? We could switch warnings -> errors in CI if not, so they're just warnings during development.

@LeaVerou
Copy link
Member

Oh I wasn't even thinking about CI. I primarily find ESLint useful as an authoring environment plugin that highlights errors and warnings inline. I would find it frustrating if it highlighted syntax errors and stylistic issues the same way.

@mAAdhaTTah
Copy link
Member

Sounds good, so we should bump warnings -> errors in CI and downgrade style issues to warnings during development.

@ExE-Boss
Copy link
Contributor Author

And how do you propose to do that?

@mAAdhaTTah
Copy link
Member

This might work. Another suggestion is to use a .js file for config and use env vars. There's a package called is-ci that could help here.

@RunDevelopment
Copy link
Member

RunDevelopment commented Mar 11, 2019

We could also check the output of eslint and throw if we got any warnings.

@LeaVerou
Copy link
Member

I saw some back and forth on dangling commas. My opinion is that if there is not near 100% consensus on a linting rule, I'd suggest not enforcing one either way. We don't need to control every single aspect of code style via linting, especially if they haven't been enforced so far at all. We don't need to provide a value for every single rule ESLint supports. Linting should help our work, not get in the way, and overly strict linting can easily get in the way.
Just my 2 cents, take it or leave it!

gulpfile.js Outdated Show resolved Hide resolved
@RunDevelopment
Copy link
Member

RunDevelopment commented Mar 30, 2019

@ExE-Boss Sorry, I did expect to be able to push to your branch. I reverted everything and made a PR.

@RunDevelopment
Copy link
Member

The only things left to discuss now are:

  1. How to handle joint variable declarations.
    Do we want to leave the tabs as is or should we add a few more vars.
    Using spaces is not possible because of "no-mixed-spaces-and-tabs": "error". Smart tabs are possible but a real pain because some formatters (e.g. the one I'm using) don't support them.
  2. The TSC hack.

.eslintrc Outdated Show resolved Hide resolved
.eslintrc Outdated Show resolved Hide resolved
Co-Authored-By: Michael Schmidt <[email protected]>
@mAAdhaTTah
Copy link
Member

Ooof, letting this lag will make it very difficult to merge. Are we all aligned on the rules presented here? I might go so far as to suggest slotting this into a potential 2.0 and introduce it in one sweeping change early on there. Thoughts?

@mAAdhaTTah mAAdhaTTah added this to the 2.0 milestone Jun 27, 2020
@LeaVerou
Copy link
Member

Using spaces is not possible because of "no-mixed-spaces-and-tabs": "error". Smart tabs are possible but a real pain because some formatters (e.g. the one I'm using) don't support them.

We should never use tabs for alignment. The codebase uses smart tabs. If we don't want to enforce them with ESLint we should avoid any rules related to indentation for now, not inserting rules that will force us to worsen how we do indentation and alignment! Again, we don't need to lint everything. We can start with a small set of rules and expand as we go.

(Also, if a tool doesn't support smart tabs, I'd consider that a bug and open an issue with them)

@RunDevelopment
Copy link
Member

RunDevelopment commented Jun 27, 2020

About alignment: Why don't we use Prettier and end the formatting debate?

@LeaVerou
Copy link
Member

Does Prettier support smart tabs?

@RunDevelopment
Copy link
Member

Just checked. It doesn't and I'd even argue that it doesn't matter. I have yet to come across a formatter that handles smart tabs correctly.
Examples: ESLint doesn't (see above). Clang format (C/C++ formatter that can also do JS) can kinda do it but it assumes that tabs are 8 spaces wide, so the alignment is all messed up with my usual 4 spaces = 1 tab. VSCode's builtin formatter can't do it.

Point is: Which formatter has good support for smart tabs? If there aren't any good formatters that support it, then I don't think this should be a criterion. Manual formatting just isn't feasible and no formatter is a lot worse than a formatter that uses, god forbid, tabs for alignment, IMO.

Don't get me wrong: I think you're correct. Tabs for indentation and spaces for alignment is the indented use for tabs and spaces but if all our automated tools work against us on this matter, it might be easiest to just not care about it.

@RunDevelopment
Copy link
Member

Closed in favor of #2831.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants