Initial port to new ESLint API by voxpelli · Pull Request #275 · standard/standard-engine

voxpelli · 2021-11-22T17:57:52Z

Fixes #234

Just throwing this out here, not thoroughly tested, but all tests pass + all types are okay

Things remaining:

Update README.md / documentation

voxpelli · 2021-11-22T22:44:40Z

I could separate out the addition of dependency-check, installed-check and npm-run-all from this PR, but they added good assistance in helping to assure that I was on the right path.

theoludwig

Thanks for taking this task! @voxpelli

Left some comments so we can discuss and improve the code.

Also: You forgot to update README.md about the changes.

bin/cmd.js

index.js

lib/resolve-eslint-config.js

theoludwig · 2021-11-23T05:58:50Z

package.json

+    "check:dependency-check": "dependency-check *.js 'bin/**/*.js' 'lib/**/*.js' --no-dev",
+    "check:installed-check": "installed-check",
+    "check:standard": "standard",
+    "check:tsc": "tsc",
+    "check": "run-p check:*",
+    "test-ci": "run-s test:*",
+    "test:tape": "tape test/clone.js test/api.js",
+    "test": "run-s check test:*"


Good idea to separate linting, tests, TypeScript check etc. (even if it could be in a separate PR as you said) 👍

We probably don't need npm-run-all dependency and test, test-ci and check scripts.

In the GitHub Actions we can run check:standard then check:tsc etc.
It makes things clearer.

I disagree that it makes things clearer + that the run-p runs things in parallel, which is faster, especially locally.

This way adding / removing / changing a command becomes very clear.

Right. 👍
We can keep run-p, I didn't know it runs in parallel.

The only thing, I think we don't need, is to have test and test-ci, it is not clear for me why we have both.

We can rename test-ci to test and remove test.
Locally, we can simply run npm run check and after npm run test or I don't mind to have a command to run both, but we can maybe name it differently than test as it runs not only test scripts but also check scripts.

Best practice is that npm test should run all of the tests of a node project.

On CI though we want to run automated tests and linting separately, as linting only needs to be run once whereas automated tests should run on a combination of platforms.

Hence why there's both test and test-ci. Since they are only two different versions of sequential runs, run-s, there's not really any duplication there.

This suggested change is not really a big deal, it's non blocking for merging, so if you rather having test and test-ci it's fine like that for now but I would still rather not having test-ci.

Best practice is that npm test should run all of the tests of a node project.

Yes, completely agree, but tests are not checks (linting, etc.).
We could separate these things.

On CI though we want to run automated tests and linting separately, as linting only needs to be run once whereas automated tests should run on a combination of platforms.

True. 👍

test/api.js

As else it checks `tmp/standard/`, which doesn't pass

voxpelli · 2021-11-29T15:10:03Z

Tweaked and fixed things here, this should now be much more ready for merge 👍

theoludwig

Great job! 😊

Nearly ready for merging, there are still some comments to be addressed.

.github/workflows/lint.yml

index.js

theoludwig · 2021-11-29T15:42:04Z

package.json

+    "check:dependency-check": "dependency-check *.js 'bin/**/*.js' 'lib/**/*.js' --no-dev",
+    "check:installed-check": "installed-check",
+    "check:standard": "standard",
+    "check:tsc": "tsc",
+    "check": "run-p check:*",
+    "test-ci": "run-s test:*",
+    "test:tape": "tape test/clone.js test/api.js",
+    "test": "run-s check test:*"


Right. 👍
We can keep run-p, I didn't know it runs in parallel.

The only thing, I think we don't need, is to have test and test-ci, it is not clear for me why we have both.

We can rename test-ci to test and remove test.
Locally, we can simply run npm run check and after npm run test or I don't mind to have a command to run both, but we can maybe name it differently than test as it runs not only test scripts but also check scripts.

voxpelli · 2021-11-29T16:35:11Z

Updated README + CHANGELOG now as well

See standard/standard#1752

theoludwig

LGTM! 🚀

There are still some comments, but they are non blocking for merging.
As it is a big BREAKING CHANGE and a quite big PR, I would rather to have at least 2 reviews approval before merging.

The complicated thing will be to migrate all the engines to this new standard-engine and also the VSCode extension vscode-standard.

I'm wondering how can we offer a smooth upgrade as this new version (v15) of standard-engine introduce lot of BREAKING things.
We could maybe do prereleases for all engines, before making it stable.

theoludwig · 2021-11-30T06:15:36Z

CHANGELOG.md

+- **BREAKING CHANGE:**  Print additional label on warnings (to separate them from errors) b7c1e17
+- **BREAKING CHANGE:** Drop support for Node 10.x. Now require ESM-compatible Node.js versions: `^12.20.0 || ^14.13.1 || >=16.0.0` #252
+- **BREAKING CHANGE:** the `parseOpts` option to the `StandardEngine` (formerly called `Linter`) constructor has been replaced with a new `resolveEslintConfig` one
+- Change: make `--verbose` the default #232


We could argue, that this is also a BREAKING CHANGE as it removes an option from the CLI and it changes the default behavior.

One could pretty much argue that bug fixes are breaking changes as well ;) Depends on what could be perceived as a public API to which one has guaranteed stability. In general I would say that CLI text output should not be treated as a public API unless explicitly stated to be for programmatic use

CHANGELOG.md

LinusU

Amazing! 🤩

Co-authored-by: Linus Unnebäck <linus@folkdatorn.se>

voxpelli · 2021-11-30T10:28:45Z

@divlo @LinusU Should we do a prerelease of 15.0.0 right away?

theoludwig · 2021-11-30T12:29:07Z

@divlo @LinusU Should we do a prerelease of 15.0.0 right away?

Yes, that would help. 👍 @voxpelli
So we can already open PRs in the multiple engines (standard, standardx, ts-standard, semistandard) using this prerelease version, as there are probably multiple things that need to change.

voxpelli · 2021-11-30T17:54:44Z

Prerelease done, update to master pending in #282

voxpelli added 2 commits November 22, 2021 15:38

Validate engine versions + dependency list

be00766

Probably make all pass

19d87a8

voxpelli requested a review from theoludwig November 22, 2021 17:57

voxpelli added 2 commits November 22, 2021 19:08

Fix failing tests

c74c572

Fix Node 12 support

b489898

voxpelli mentioned this pull request Nov 22, 2021

Usage of deprecated APIs of ESLint #234

Closed

voxpelli marked this pull request as ready for review November 22, 2021 22:44

voxpelli changed the title ~~Initial port to new api~~ Initial port to new ESLint API Nov 22, 2021

theoludwig suggested changes Nov 23, 2021

View reviewed changes

voxpelli added 8 commits November 29, 2021 14:49

Remove leftover todo comment

01c665e

Improve naming of "results" post refactoring

a21da9c

Extract hasErrors and hasWarnings variables

6032cc4

Add coverage report

ca655e0

Exclude tests from type checks

8c520ef

As else it checks `tmp/standard/`, which doesn't pass

Tweak type format

ac1532a

Extract utility methods

1188847

Remove non-needed @ts-ignore

e885367

voxpelli added this to the 15.0.0 milestone Nov 29, 2021

voxpelli added 2 commits November 29, 2021 15:59

Add unit tests for utils

528cb41

We need to ensure we run the clone test first

c001edf

voxpelli requested review from LinusU and theoludwig November 29, 2021 15:09

theoludwig suggested changes Nov 29, 2021

View reviewed changes

voxpelli added 3 commits November 29, 2021 16:51

Rename build to lint

e55d23f

Rename Linter to StandardEngine

378be51

Update README + CHANGELOG

5017766

voxpelli requested a review from theoludwig November 29, 2021 16:35

Sync dependabot.yml with standard/standard repo

740d61f

See standard/standard#1752

voxpelli mentioned this pull request Nov 29, 2021

build(deps): bump actions/setup-node from 2.4.1 to 2.5.0 #280

Closed

theoludwig approved these changes Nov 30, 2021

View reviewed changes

voxpelli requested a review from feross November 30, 2021 06:58

LinusU reviewed Nov 30, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

LinusU approved these changes Nov 30, 2021

View reviewed changes

voxpelli and others added 2 commits November 30, 2021 11:05

Update CHANGELOG.md

b75d9cd

Co-authored-by: Linus Unnebäck <linus@folkdatorn.se>

Merge remote-tracking branch 'origin/master' into port-to-new-api

d34f746

voxpelli merged commit 5f66f04 into master Nov 30, 2021

voxpelli deleted the port-to-new-api branch November 30, 2021 10:27

MichaelDeBoey mentioned this pull request Jan 31, 2022

feat: support ESLint 8.X standard/standard#1725

Merged

26 tasks

Uh oh!

Conversation

voxpelli commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voxpelli commented Nov 22, 2021

Uh oh!

theoludwig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theoludwig Nov 23, 2021

Choose a reason for hiding this comment

Uh oh!

voxpelli Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

theoludwig Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

voxpelli Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

theoludwig Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

voxpelli commented Nov 29, 2021

Uh oh!

theoludwig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

theoludwig Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

voxpelli commented Nov 29, 2021

Uh oh!

theoludwig left a comment

Choose a reason for hiding this comment

Uh oh!

theoludwig Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

voxpelli Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LinusU left a comment

Choose a reason for hiding this comment

Uh oh!

voxpelli commented Nov 30, 2021

Uh oh!

theoludwig commented Nov 30, 2021

Uh oh!

voxpelli commented Nov 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

voxpelli commented Nov 22, 2021 •

edited

Loading

theoludwig Nov 29, 2021 •

edited

Loading

theoludwig Nov 29, 2021 •

edited

Loading