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

Suggestion: prefer eslint syntax over style #589

Closed
SleeplessByte opened this issue Jan 21, 2019 · 7 comments · Fixed by #645
Closed

Suggestion: prefer eslint syntax over style #589

SleeplessByte opened this issue Jan 21, 2019 · 7 comments · Fixed by #645
Labels
chore 🔧 Meta related task such as build, test, linting, maintainers.json etc. documentation 📖 Documentation changes

Comments

@SleeplessByte
Copy link
Member

SleeplessByte commented Jan 21, 2019

With more and more tools becoming available to automatically format the code base to a certain set of style rules (and more comprehensive than eslint --fix, I'm also more inclined to stop teaching style preference, as indicated by mentor notes, mentor guidance docs and discussions on slack.

The current state

Currently, the javavscript package.json sets a very restrictive code style (airbnb) and I don't think this:

  • helps the student understand the language
  • helps the student get fluent in a language
  • helps the mentor mentoring as downloading exercises can result in wibbly wobblies all over the place

We don't instruct or enforce these rules (#44 (comment)) strictly, but I seem them in mentoring comments and most IDE's will run these automagically if present.

Recommendation

I therefore recommend to drop down to eslint:recommend or if we must have a non-company style guide, use standard with semicolon rules disabled (people should decide about ASI themselves -- I don't think pushing people who know about ASI helps anyone, and TS doesn't have them either).

If this is something we want, I think doing it sooner rather than later is good and will speed up #480 greatly. The eslintignore list is still quite long and I don't think tháts particularly helpful.

As a final note, I think we should better communicate to the students to run npm/yarn install and then npm/yarn test. I also suggest running test followed by lint so that someone is only bugged once the test succeeds, instead of using a "pretest": "lint":

{ 
  "scripts": {
    "test": "test:jest && test:lint",
    "test:jest": "...",
    "test:lint": "..."
   }
}
@tejasbubane
Copy link
Member

Currently, the javavscript package.json sets a very restrictive code style (airbnb) and I don't think this:

  • helps the student understand the language

  • helps the student get fluent in a language

  • helps the mentor mentoring as downloading exercises can result in wibbly wobblies all over the place

The eslint config is not for students. It is for contributors contributing to this track repo. We do not enforce any style restriction on students.

@SleeplessByte
Copy link
Member Author

SleeplessByte commented Jan 21, 2019

I know that we don't, but that's not how IDE's work. VS Code for example will pick up the eslint configuration by default if the plugin is active. Atom en Webstorm too!

Evidently, we should change something as mentors áre referring to it in their comments :)
@tejasbubane I don't know of a super smart way to do this right; running eslint:recommended for students is actually something I would prefer since it helps you fix actual bugs --

@tejasbubane
Copy link
Member

Ok. Some very valid points that I completely took for granted:

I know that we don't, but that's not how IDE's work.

I did not realize that we give the eslint config in exercises which students download. I assumed we only validate it from main package.json file

Evidently, we should change something as mentors áre referring to it in their comments :)

I did not know this as well. Looks like we definitely should do something about this.

I am in the favor of not recommending anything to students - only use it for CI in our repo. For mentors, put a mentor note regarding the same. What do you think?

@tejasbubane
Copy link
Member

tejasbubane commented Jan 21, 2019

I am in the favor of not recommending anything to students - only use it for CI in our repo. For mentors, put a mentor note regarding the same. What do you think?

This is because JS does not have a standard formatting tool (you mentioned the same in previous comment). Had it been golang or elixir, it would be good to recommend.

Having said that I am OK with using eslint:recommend as well - since it is less strict and opinionated that airbnb. But I would certainly put up a mentor note to atleast not make it a requirement for students (it is fine to subtly suggest, but not ok to make it a requirement for approving solutions).

@SleeplessByte
Copy link
Member Author

SleeplessByte commented Jan 21, 2019

I'm happy to actually strip all style items left in eslint:recommended btw! We are definitely in sync on reasoning and I agree with everything you say.

@SleeplessByte
Copy link
Member Author

@tejasbubane I think we can choose a permutation one of these:

  • use a track-level eslint configuration on something "strict", eslint:recommended including style rules,
  • drop exercise-package.json eslint config and dependencies. This will not disable eslint in travis in the exercises folders, if I'm right.
  • change exercise-package.json eslint and dependencies to use eslint:recommended or even looser.

@SleeplessByte SleeplessByte added documentation 📖 Documentation changes chore 🔧 Meta related task such as build, test, linting, maintainers.json etc. labels Jan 21, 2019
@tejasbubane
Copy link
Member

use a track-level eslint configuration on something "strict", eslint:recommended including style rules,

👍

drop exercise-package.json eslint config and dependencies. This will not disable eslint in travis in the exercises folders, if I'm right.

I am OK with this. I would also recommend removing the eslint config from package.json into .eslintrc file instead.

We have a CI check to ensure package.json of all exercises are exactly the same as the one in root path. We might need to change that a little to just check consistency across all tracks - but leave the root file alone.

tejasbubane pushed a commit that referenced this issue Mar 25, 2019
* Upgrade eslint and extract config

- upgrade eslint to 5.15.3
- upgrade eslint-plugin-import to 2.16.0
- correctly use the import plugin
- remove linebreak-style (fixes #502)
- use eslint:recommended (fixes #589, fixes #480)

In a later change we should turn on airbnb-base for maintaining via a script that switches this.

In a later change, we should turn on several rules that guard against potential bugs, such as `no-shadow` `no-undefined` `no-var`; additionally we can add stylistic consistency without forcing style choises such as consistent spacing (but not required).

* Saving files before refreshing line endings

* Normalize all the line endings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore 🔧 Meta related task such as build, test, linting, maintainers.json etc. documentation 📖 Documentation changes
Projects
None yet
2 participants