Add more JSDoc annotations + validate them and refactor to make them clearer #248

voxpelli · 2021-08-14T18:18:23Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[ ] New feature
[x] Other, please explain: To make #234 simpler to solve

What changes did you make? (Give an overview)

The main changes:

I extended the JSDoc types + added a validation of them using the TypeScript cli utility – this to better ensure that the public API contract doesn't get broken when I, @divlo or someone else new to the code contributes.
In the process of adding the types I found that parseOpts() was a bit all over the place and unlike its name was actually mainly focused on simply constructing the needed ESLint Config. I therefore revamped it to be fully focused on that + extracted it into a file of its own to make both it and the main file shorter and more focused.
As a result of the revamping of parseOpts() and adding types, the custom parseOpts argument added in Stop package.json settings from overriding explicit options #146 also had to get a modification to it (or be removed – I searched through the entirety of GitHub and there's no public usage there of it at least). I replaced it with a new resolveEslintConfig argument which mimics the original one.

Is there anything you'd like reviewers to focus on?

Whether the general approach is good and where we want to take this
Actually test this with standard itself

Why is this a draft PR?

I haven't actually pulled in this version into standard itself and tried it there yet, wanted to get this up for some more eyes first, as eg. @divlo has been working in this area as well.

welcome · 2021-08-14T18:18:24Z

🙌 Thanks for opening this pull request! You're awesome.

voxpelli · 2021-08-14T18:21:33Z

With these types it would also be possible to generate .d.ts files from this code, if we would like to publish with types.

Also – I failed to mention – the type strictness is pretty much to the max right now, because that's how I like it so that was what I aimed for. If others finds that annoyingly strict, then we can absolutely relax them – now or later.

theoludwig

Whether the general approach is good and where we want to take this

I also like to add types to my JavaScript, I don't write JavaScript anymore for my personal project, whenever I can, I use TypeScript.
With this PR, we are nearly like using TypeScript and in fact, we are installing it to check the JSDoc types.

I would argue that we should choose the right tool for the right job, so if we want to add more types, it's probably better to transition the entire project to TypeScript, that would make more sense in my opinion.

I would be 100% agree to do this, but that would be a major change for the project, and it is more a decision to take from the creator of this original codebase: @feross, @Flet, @LinusU.

Actually test this with standard itself

Yes, we should definitely test that standard/standard don't break with this major change. 👍

I searched through the entirety of GitHub and there's no public usage there of it at least

That still means, it is a BREAKING CHANGE, even if no one is actually using it.

Even if I don't agree with the current implementation (as the implementation should use TypeScript in my opinion), thank you for taking this PR, it will definitely make #234 simpler to solve, and the main idea behind this PR is great. 😄

bin/cmd.js

index.js

lib/resolve-eslint-config.js

tsconfig.json

index.js

voxpelli · 2021-08-14T21:52:46Z

Whether the general approach is good and where we want to take this

I also like to add types to my JavaScript, I don't write JavaScript anymore for my personal project, whenever I can, I use TypeScript.
With this PR, we are nearly like using TypeScript and in fact, we are installing it to check the JSDoc types.

I would argue that we should choose the right tool for the right job, so if we want to add more types, it's probably better to transition the entire project to TypeScript, that would make more sense in my opinion.

I would be 100% agree to do this, but that would be a major change for the project, and it is more a decision to take from the creator of this original codebase: @feross, @Flet, @LinusU.

We actually had JSDoc types before, but they were not validated and was hence quite incomplete. So basically what I mostly did was to add validation of those JSDoc comments, to help me complete them and to help us from avoiding regressions in them.

On TypeScript – it essentially consists of three parts – the validator, the type compiler and the code compiler – and one can use the validator and type compiler on JS code just as well as one can on TS code, but of course the code compiler is of no use for JS code as JS is already JS.

For a project of this size and complexity I would say that adding in a transpiler of any kind, TypeScript or otherwise, would be quite overkill and add nothing extra than what a JSDoc annotated JS gives 🙂 Except for a hard dependency on a transpiler step.

I searched through the entirety of GitHub and there's no public usage there of it at least

That still means, it is a BREAKING CHANGE, even if no one is actually using it.

Good point, I very much meant to write BREAKING CHANGE, it indeed is and since this is intended to target 15.x it feels ok.

I'm do am wondering though whether it makes most sense to migrate the API to a new one or whether just dropping it would be better, when we know of no current use cases.

theoludwig

Overall, it Looks Good To Me. 👍
But I would rather have a second review approval before merging this.
Also as you said, we should test this with standard itself.

We actually had JSDoc types before, but they were not validated and was hence quite incomplete. So basically what I mostly did was to add validation of those JSDoc comments, to help me complete them and to help us from avoiding regressions in them.

You actually changed my mind. 😄
You are not wrong there, we were already using JSDoc comments, so why not add some extra checks for the CI to avoid regressions in the future. 👍
But I still think a better implementation would be to migrate the project completely to TypeScript.

For a project of this size and complexity I would say that adding in a transpiler of any kind, TypeScript or otherwise, would be quite overkill and add nothing extra than what a JSDoc annotated JS gives slightly_smiling_face Except for a hard dependency on a transpiler step.

The real ask there is: when is the size of the project sufficient, to use TypeScript ?
If the project becomes bigger and bigger, it will be even harder to migrate the project to TypeScript, right?
So I would say, it is not right in my opinion and TypeScript is the right tool no matter the size of the project.

The main advantages to use TypeScipt is the better syntax to do the same thing :

/** type {string[]} */
const result = []

vs

const result: string[] = []

/**
 * @typedef StandardCliOptions
 * @property {import('../').linter} [linter]
 * @property {string} [cmd]
 * @property {string} [tagline]
 * @property {string} [homepage]
 * @property {string} [bugs]
 */

vs

import type { linter as Linter } from '../'

interface StandardCliOptions {
  linter: Linter
  cmd: string
  tagline: string
  homepage: string
  bugs: string
}

README.md

LinusU

Looks good 👍

Is this intended to go out as a major change, sine parseOpts was changed to resolveEslintConfig?

voxpelli · 2021-08-16T13:56:52Z

Is this intended to go out as a major change, sine parseOpts was changed to resolveEslintConfig?

Yes, I've started a milestone for a 15.0.0 release, thinking we do this + some of the other stuff mentioned in #234 + the #234 fix itself (I guess me, @divlo or someone else can extract other stuff into standalone issues – like the "move to Promises", "move to ES6 class")

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

theoludwig · 2021-08-16T14:18:28Z

(I guess me, @divlo or someone else can extract other stuff into standalone issues – like the "move to Promises", "move to ES6 class")

Yep, we better separate this in multiple issues/PRs, as it is quite a big change. 😄
New issues availables: #249, #250

Yes, I've started a milestone for a 15.0.0 release

@voxpelli This release also includes #232 (already been merged).
Do you think, we do #234, #249, #250 and we release a new version or we can also include #231 in v15 ?
I mean we could make standard@17 a big release.

voxpelli · 2021-08-16T14:23:37Z

Yes, I've started a milestone for a 15.0.0 release

@voxpelli This release also includes #232 (already been merged).
Do you think, we do #234, #249, #250 and we release a new version or we can also include #231 in v15 ?
I mean we could make standard@17 a big release.

Makes sense to add #231 in there as well, I added it to the milestone now 👍, had totally missed reading up on that one.

Slight wish: That we merge this PR first 😛

theoludwig · 2021-08-17T12:42:17Z

test/api.js

@@ -43,15 +43,15 @@ test('api: lintTextSync', function (t) {
 test('api: parseOpts -- avoid self.eslintConfig parser mutation', function (t) {


Maybe we can rename this test, to use resolveEslintConfig instead of parseOpts reference.
(Same thing for line 51).

theoludwig · 2021-08-17T12:48:06Z

Slight wish: That we merge this PR first 😛

Sure, it makes sense!
Is there is anything missing to merge this PR and to make it "Ready for review" instead of "Draft"? @voxpelli

Once this PR has been merged, I'm happy to work on these issues: #250 and #251
I also opened a new PR to drop Node.js v10 support: #252

Feel free to take #234 and #249, it would be awesome if you could tackle them! Thanks! 😄

Actually test this with standard itself

Be aware that there is already a test for that and it is already passing (see: test/clone.js).

voxpelli · 2021-08-18T10:20:22Z

Actually test this with standard itself

Be aware that there is already a test for that and it is already passing (see: test/clone.js).

Did not know! Let's merge then :)

welcome · 2021-08-18T10:21:41Z

🎉 Congrats on getting your first pull request landed!

refactor: add more jsdoc + validate jsdoc

a1f3a07

voxpelli added this to the 15.0.0 milestone Aug 14, 2021

voxpelli requested review from feross, LinusU and theoludwig August 14, 2021 18:18

voxpelli self-assigned this Aug 14, 2021

theoludwig suggested changes Aug 14, 2021

View reviewed changes

voxpelli added 4 commits August 14, 2021 23:18

Remove the comments I so sloppily left there

ec94494

Tweak tsconfig comments

6627cef

Remove mistakenly added empty line

88b8c7c

Update bin/cmd.js

8d3b829

voxpelli requested a review from theoludwig August 15, 2021 14:55

theoludwig approved these changes Aug 15, 2021

View reviewed changes

LinusU reviewed Aug 16, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

LinusU approved these changes Aug 16, 2021

View reviewed changes

Update README.md

0bcb1f1

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

theoludwig reviewed Aug 17, 2021

View reviewed changes

voxpelli marked this pull request as ready for review August 18, 2021 10:19

voxpelli merged commit 84f2a37 into master Aug 18, 2021

voxpelli deleted the refactor/types branch August 18, 2021 10:21

theoludwig mentioned this pull request Aug 18, 2021

feat: usage of ES6 class syntax #253

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more JSDoc annotations + validate them and refactor to make them clearer #248

Add more JSDoc annotations + validate them and refactor to make them clearer #248

voxpelli commented Aug 14, 2021 •

edited

Loading

welcome bot commented Aug 14, 2021

voxpelli commented Aug 14, 2021

theoludwig left a comment •

edited

Loading

voxpelli commented Aug 14, 2021

theoludwig left a comment

LinusU left a comment

voxpelli commented Aug 16, 2021 •

edited

Loading

theoludwig commented Aug 16, 2021

voxpelli commented Aug 16, 2021

theoludwig Aug 17, 2021

theoludwig commented Aug 17, 2021 •

edited

Loading

voxpelli commented Aug 18, 2021

welcome bot commented Aug 18, 2021

		@@ -43,15 +43,15 @@ test('api: lintTextSync', function (t) {
		test('api: parseOpts -- avoid self.eslintConfig parser mutation', function (t) {

Add more JSDoc annotations + validate them and refactor to make them clearer #248

Add more JSDoc annotations + validate them and refactor to make them clearer #248

Conversation

voxpelli commented Aug 14, 2021 • edited Loading

welcome bot commented Aug 14, 2021

voxpelli commented Aug 14, 2021

theoludwig left a comment • edited Loading

Choose a reason for hiding this comment

voxpelli commented Aug 14, 2021

theoludwig left a comment

Choose a reason for hiding this comment

LinusU left a comment

Choose a reason for hiding this comment

voxpelli commented Aug 16, 2021 • edited Loading

theoludwig commented Aug 16, 2021

voxpelli commented Aug 16, 2021

theoludwig Aug 17, 2021

Choose a reason for hiding this comment

theoludwig commented Aug 17, 2021 • edited Loading

voxpelli commented Aug 18, 2021

welcome bot commented Aug 18, 2021

voxpelli commented Aug 14, 2021 •

edited

Loading

theoludwig left a comment •

edited

Loading

voxpelli commented Aug 16, 2021 •

edited

Loading

theoludwig commented Aug 17, 2021 •

edited

Loading