ESLint, cross-env make, run all tests in 2 minutes (instead of 20) #276

SleeplessByte · 2019-07-24T17:57:03Z

⚠⚠ missing README.md update. Will make that change after review ⚠⚠

Before you review

🛑 This is a large PR. Read the first commit to see the actual changes (small). The rest is implementation and making sure everything is consistent. I had to fix a few thousand linting issues (that ALREADY existed in the current codebase, but wasn't being checked).

a2df205 has the most significant changes. The nicer messages and some further make fixes are towards the end of the PR commits. It took a while to get all the CIs working. I would like to squash-merge this once it's completely ready.

`lint` changes

Switches linting to eslint, preserving most of the rules set-up for tslint before, but allowing for 2 different configurations:

common holds student configuration. This is what they pull down, and if their IDE supports it, or if they run yarn lint, this is what is used. The configurations have been loosened quite a bit for students, in order to only catch bugs, but not dictate any style. Before, there were quite a few stylistic choices in tslint set-up, which have been removed.
maintaining holds track configuration/configuration for maintainers. This is stricter and used by the ci and make file.

Indentation

The indentation has been normalised. A big chunk of the files was indented 2 spaces and another chunk 4 spaces. Sometimes it was only the tests and sometimes only the example. I chose the rule that matches javascript, which uses 2 spaces. In the next commit you'll see the update.

`make` changes

Updated the make file to run the lintreport from eslint, using the track package, as well as updating them for cross-env:

Use quotes around file paths so that paths with spaces don't break make
Use consistent hyphenated naming, instead of sometimes camelCase and sometimes dash-case
Speed up the tests by 2x to 1000x, depending on the IO speed, by symlinking node_modules instead of re-installing for EVERY test.

Fixes #240
Closes #267
Closes #268
Closes #269
Closes #270
Closes #271
Closes #272
Closes #273
Closes #274
Closes #275

Switches linting to eslint, preserving most of the rules set-up for tslint before, but allowing for 2 different configurations: - common holds student configuration. This is what they pull down, and if their IDE supports it, or if they run yarn lint, this is what is used. - maintaining holds track configuration/configuration for maintainers. This is stricter and used by the ci and make file. The configurations have been loosened quite a bit for students, in order to only catch bugs, but not dictate any style. Before, there were quite a few stylistic choices in tslint set-up. The identation has been normalised. A big chunk of the files was indented 2 spaces and another chunk 4 spaces. Sometimes it was only the tests and sometimes only the example. I chose the rule that matches javascript, which uses 2 spaces. In the next commit you'll see the update. Updated the make file to run the lintreport from eslint, using the track package, as well as updating them for cross-env: - Use quotes around file paths so that paths with spaces don't break make - Use consistent hyphenated naming, instead of sometimes camelCase and sometimes dash-case - Speed up the tests by 2x to 1000x, depending on the IO speed, by symlinking node_modules instead of re-installing for EVERY test. Fixes #240 Closes #267 Closes #268 Closes #269 Closes #270 Closes #271 Closes #272 Closes #273 Closes #274 Closes #275

This syncs all the configuration, updates all the files for the new styling rules and makes sure tests still pass. This commit _purposefully_ doesn't fix ALL linting, so that it reports on issues via danger (if that's changed correctly).

SleeplessByte · 2019-07-24T18:15:22Z

Also:

~~circle ci runs a non-current verison of node and must be fixed~~ fixed
~~travis is failing for no good reason (most likely some files not being copied over correctly / linked)~~ fixed to report danger only
appveyor is working as expected and failing on exactly what I wanted it to fail.

Stub files often don't include a return type, or accessibility modifiers. That's okay, because we don't force them upon the student -- our track strict lint does enforce it.

Adding a return type will guide the student that they have to add the return keyword.

pacman-bot · 2019-07-24T19:07:14Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

Moves all the testing to a single directory, removing the need to install / symlink the dependencies for each (80+) exercise. Additionally, shows current progress but overwrites "similar" messages when possible.

It is not available on all systems. yes n | cp -i might be an alternative, but we don't need it because the make file has changed and -n is no longer needed.

masters3d

I think this or needs to be split up into at least 3. 1) upgrade pass 2) eslint pass ( just updating the lint condiga ) 3) ci changes and others

Looking good though.

masters3d · 2019-07-25T00:52:57Z

common/package.json

+    "test": "yarn lint:types && jest --no-cache",
+    "lint": "yarn lint:types && yarn lint:ci",
+    "lint:types": "yarn tsc --noEmit -p .",
+    "lint:ci": "eslint . --ext .tsx,.ts"


What about just passing in the strict here ?

eslint -c ~/my-eslint.json

This is the parity with the old tslint call.

-c passes in a configuration file (instead of looking in the directory for one). It doesn't "fix" the extensions.

Edit

I think you mean pass in the strict configuration here. I really don't want to pass the strict configuration down to the student in the first place and don't want to pollute scripts with stuff they can't run.

I think if the goal is to:

have all the scripts available to the student

have a separate lint config for ci

...then this is a possible solution. We did it similarly in the JavaScript track and it works well.

As a new contributor how would I run the strict lint check locally?

Either run as is on everything:

make test

Or run 1 exercise:

ASSIGNMENT=bob make test-assignment

Will update README.md and I have a WIP CONTRIBUTING.md to aid with this.

masters3d · 2019-07-25T00:56:34Z

common/package.json

-    "typescript": "^2.5.3"
+  "scripts": {
+    "test": "yarn lint:types && jest --no-cache",
+    "lint": "yarn lint:types && yarn lint:ci",


This lint should be the use config lint, not the ci. Presumably the ci lint and user lint have different rules

ci lint and user lint have "different rules". It's just named that by convention.

(If you think the name should be changed because it's confusing, please provide a suggestion 👍. I'm not married to the name 😄 )

Instead of doing the magic of different lint rule checking in the make file, you can define that work away by just having two distinct lint commands. One that gets run for on the CI only which would have a non standard configuration file name and the other will be the the default standard name config which would be the default.

I absolutely no longer understand what you mean.

common/package.json has lint which will run two linters: compile linting (via tsc, called lint:types, but it's just a name) and eslint linting (called lint:ci, but it's just a name). Both allow for watching, and this allows students to watch on either.

They can watch on tsc if they don't use a supported IDE that does it for them, whilst writing code

They can watch on eslint once they are ready to submit

They can choose not to lint at all, all of this is optional

When linting by a student, it will use common/.eslintrc.

maintaining/.eslint.track.rc is the eslint configuration that is used by the CI/us maintainers/the make file. Because everything else is equal (it still needs to check types, which uses tsconfig.json , which we can also provide separately (read: stricter version for ci/make/maintainers), it can re-use yarn lint which will call yarn lint:types and yarn lint:ci.

What you call lint:types is not a linter. That is the typescript type checker. That one needs a different name. Maybe typechecker. We needed because jest doesn’t types check so a test can fail because of a type error.

The lint was meant to be used by users but previously the rules were the same

lintci was different because we don’t want the lint to error out on the ci but we do want it to report the error so danger can pick them up and report on PRs.

So now we are introducing a ESLint. So we need eslint to create the violation json file when running on the ci (lintci) So now we have to parse that json so danger picks them up. I don’t thinking fixing those violation should be part of this pr.

The regular lint command (lint) we don’t want the json file and since this is running on the local dev machine it should error or warn which the ide would pick up.

I think we're partially on the same wave length and partially completely missing each other's points/concerns 😓

Edit:

I've edited this comment to be more constructive and tried to list concisely what I think and what the state is. I hope this clears up the confusion.

lintci (old package.json) is not the same as lint:ci this one

lint:ci is not used to generate the report for danger. It wasn't before and it still isn't

If you would like other names for lint:types and lint:ci, please propose them, because commentary that just shits on it doesn't really help us here. I took these names because I've seen and used them before. I don't mind to change them, but naming is hard, so I'd like that to be a team effort.

eslint is already generating a report and it's also already integrated with Danger. In other words, this PR already holds the changes to make that work!

The regular lint command(yarn lint) for students calls both thetscandeslint` commands ánd errors visually, both in the IDE and the CLI if something is wrong. It's not generating a JSON report.

I really would like to try to keep out any maintainers / remote ci stuff out of the common/package.json.

SleeplessByte · 2019-07-25T01:14:06Z

I think this or needs to be split up into at least 3. 1) upgrade pass 2) eslint pass ( just updating the lint condiga ) 3) ci changes and others

I understand that review would be easier if it were split up, but unfortunately doing those three steps leads to a lot of unnecessary and throw-away work, most of which caused by the hardcoded tslint in the CI, and the fact that we would have a breaking repository in the intermediate state.

I therefore feel that this can't reasonably be split up nor should be. It was about 8 hours of work to do it like this, and it's quite frustrating to read the "oh yeah great but split up" commentary as it completely disregards the vast amount of work it would take to do that.

Because circleci

masters3d · 2019-07-25T17:18:59Z

exercises/accumulate/accumulate.test.ts


-    xit('accumulate recursively', () => {
-        const result = accumulate('a b c'.split(/\s/), (char: string) => accumulate('1 2 3'.split(/\s/), (digit: string) => char + digit))
+  it('accumulate recursively', () => {


Only the first should be enabled.
#88

masters3d · 2019-07-25T17:26:39Z

Can you split in into two PRs? The first with all the config changes but leaves all the sample code as is. Then once that is merged, one more with all the code sample changes?

SleeplessByte · 2019-07-25T17:43:02Z

@masters3d yes! That I can do :)

SleeplessByte · 2019-07-29T23:11:44Z

It's coming, just need to find a spare moment!

SleeplessByte added 2 commits July 24, 2019 19:49

Implement changes of eslint

95267d0

This syncs all the configuration, updates all the files for the new styling rules and makes sure tests still pass. This commit _purposefully_ doesn't fix ALL linting, so that it reports on issues via danger (if that's changed correctly).

SleeplessByte self-assigned this Jul 24, 2019

Use package tsc

fe2fdfe

SleeplessByte added 6 commits July 24, 2019 21:04

Fix final files

0318209

Only lint .example and .test

85921ab

Stub files often don't include a return type, or accessibility modifiers. That's okay, because we don't force them upon the student -- our track strict lint does enforce it.

Make two-fer easier

7f8dd34

Adding a return type will guide the student that they have to add the return keyword.

Remove tslint.json

e60c311

Use travis to run danger

51b275e

Now that travis is danger-only, it should only have one version

2a6baf0

SleeplessByte added 7 commits July 24, 2019 21:10

Break example to test danger

a9dd70d

Make whitespace resistant

38cca01

Use one directory, add commentary and colours

48c248d

Moves all the testing to a single directory, removing the need to install / symlink the dependencies for each (80+) exercise. Additionally, shows current progress but overwrites "similar" messages when possible.

Cleanup final messages

3f9b0e6

Fix danger

9ff4233

Remove -n flag

66daaee

It is not available on all systems. yes n | cp -i might be an alternative, but we don't need it because the make file has changed and -n is no longer needed.

Fix danger again

9587636

SleeplessByte changed the title ~~Chore/eslint make qol~~ ESLint, cross-env make, run all tests in 2 minutes (instead of 20) Jul 25, 2019

SleeplessByte added 3 commits July 25, 2019 02:51

Use right variable

f798cf0

Use relative paths

889d75e

Danger works, fix exercise

ca88257

masters3d reviewed Jul 25, 2019

View reviewed changes

SleeplessByte added 4 commits July 25, 2019 03:03

Use dangerfile dsl

8f6697e

Try to force install LTS node

f787100

Print all the files

330968c

More printing

50a6dd2

SleeplessByte added 2 commits July 25, 2019 03:17

Does this give a non-relative path?

dd055f8

Hardcode travis path

38607c4

SleeplessByte removed the do not merge 🚧 label Jul 25, 2019

SleeplessByte added 8 commits July 25, 2019 03:26

Fix syntax error

8d90b0c

Fix for older ruby

7d625b0

load nvm onto path

d44e2ba

Try nvm as one command

5e48411

Try brew for node

6bcbf13

Yay. Upgrade node via brew

46521a0

macosx support

12e9612

Don't copy node_modules

406b3b9

Because circleci

SleeplessByte requested review from crivasgomez and masters3d July 25, 2019 02:29

masters3d reviewed Jul 25, 2019

View reviewed changes

masters3d mentioned this pull request Aug 15, 2019

Split of PR #276 #279

Closed

masters3d removed the request for review from crivasgomez August 16, 2019 17:55

masters3d merged commit 6382524 into exercism:master Aug 16, 2019

SleeplessByte deleted the chore/eslint-make-qol branch August 16, 2019 17:57

Cerber-Ursi mentioned this pull request Oct 11, 2019

Inconsistent versions of Eslint and typescript plugin #287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESLint, cross-env make, run all tests in 2 minutes (instead of 20) #276

ESLint, cross-env make, run all tests in 2 minutes (instead of 20) #276

SleeplessByte commented Jul 24, 2019 •

edited

Loading

SleeplessByte commented Jul 24, 2019 •

edited

Loading

pacman-bot commented Jul 24, 2019 •

edited

Loading

masters3d left a comment

masters3d Jul 25, 2019

SleeplessByte Jul 25, 2019 •

edited

Loading

masters3d Jul 25, 2019

SleeplessByte Jul 25, 2019 •

edited

Loading

masters3d Jul 25, 2019

SleeplessByte Jul 25, 2019 •

edited

Loading

SleeplessByte Jul 25, 2019

masters3d Jul 25, 2019

SleeplessByte Jul 25, 2019

masters3d Jul 25, 2019

SleeplessByte Jul 25, 2019 •

edited

Loading

SleeplessByte commented Jul 25, 2019 •

edited

Loading

masters3d Jul 25, 2019

masters3d commented Jul 25, 2019

SleeplessByte commented Jul 25, 2019

SleeplessByte commented Jul 29, 2019

ESLint, cross-env make, run all tests in 2 minutes (instead of 20) #276

ESLint, cross-env make, run all tests in 2 minutes (instead of 20) #276

Conversation

SleeplessByte commented Jul 24, 2019 • edited Loading

Before you review

lint changes

Indentation

make changes

SleeplessByte commented Jul 24, 2019 • edited Loading

pacman-bot commented Jul 24, 2019 • edited Loading

masters3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SleeplessByte Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SleeplessByte Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SleeplessByte Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SleeplessByte Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

SleeplessByte commented Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

masters3d commented Jul 25, 2019

SleeplessByte commented Jul 25, 2019

SleeplessByte commented Jul 29, 2019

SleeplessByte commented Jul 24, 2019 •

edited

Loading

`lint` changes

`make` changes

SleeplessByte commented Jul 24, 2019 •

edited

Loading

pacman-bot commented Jul 24, 2019 •

edited

Loading

SleeplessByte Jul 25, 2019 •

edited

Loading

SleeplessByte Jul 25, 2019 •

edited

Loading

SleeplessByte Jul 25, 2019 •

edited

Loading

SleeplessByte Jul 25, 2019 •

edited

Loading

SleeplessByte commented Jul 25, 2019 •

edited

Loading