Refactoring generation script #135

Julien-R44 · 2022-10-03T00:46:58Z

Hey 👋

So as discussed in #134, here is my refactoring attempt. There is a lot of stuff missing, especially tests, and still some issues with the script, but I am on it. I'll try to finish it this week, but I'll be pretty busy at work. So at worst, this weekend I hope

Migration from esno to tsx because esno is kind of depreciated, and is now just an alias to tsx
Added a very simple argument parsing system to be able to launch the script for one or more plugins. Could be interesting to replace by something like yargs in the future ? Can be run like : pnpm generate:rules --plugins=vue-pug,node,eslint
Automatic removal of unnecessary comments left by the generation library json-schema -> typescript
Modification of the output in CLI for something a bit sexier 😋
And finally, a refacto. I split the whole thing up quite a bit. In the end we have more lines of code than before, but in my opinion much more readable.
( I forgot to modify the visibility of all the methods of the RuleFile class, it's just an oversight, I'll fix it )

So I'm opening this PR as a draft so that you can tell me if anything doesn't suit you, so we can discuss it, and I can make the necessary changes!

Thanks!

Shinigami92 · 2022-10-03T05:21:24Z

Modification of the output in CLI for something a bit sexier 😋

Holy shit, this is awesome 🤯

Will start to look into it in some hours 🚀

package.json

Shinigami92

Oh my god, your code is so clean 🥹

I have some opinionated caveats here and there, but even tend to address them in separate further PRs, as I'm using squash-merge for PRs

scripts/generate-rule-files/index.ts

Julien-R44 · 2022-10-03T17:21:20Z

Glad you like it man! 🎉🎉
I wish I could give you access to edit the PR, but I still haven't figured out how

package.json

Co-authored-by: Shinigami <[email protected]>

Julien-R44 · 2022-10-03T21:34:05Z

I think we're pretty good !

I fixed the last generation bugs
I added some basic tests with snapshots
When I restart a complete generation, we have almost the same files. GLobally the only differences are just some extra spaces in some files. Example :

Apart from that, we have other diffs but I guess they are coming from files that you edited manually because there are interfaces in them.

The only thing a bit annoying is the lint errors because of the @typescript-eslint/typedef rule. Would you be ok if we deactivate it? IMO we should take full advantage of the automatic inference of typescript on this, otherwise, it pollutes the codebase a lot, for little added value, I think. Let me know your position on this

On the other hand, having to type explicitly the return of the functions, I agree with that

Let me know if anything looks wrong to you !

Julien-R44 · 2022-10-03T22:32:30Z

Okay so I had to do a temporary fix. We had some type errors on @poppinss/colors that is a sub-dependency of @poppinss/cliui ( the tool that allows us to have a slick output on the generation script )

So I overrided this dependency in the package.json, field pnpm.overrides with a branch I made temporarily on the @poppinss/colors repo.

Once my PR is merged ( poppinss/colors#8 ), we will just have to remove the override and update @poppinss/colors. I'll do a new PR here when that happens !

package.json

Shinigami92 · 2022-10-04T06:40:02Z

scripts/generate-rule-files/index.ts

+ const rules = Object.entries(plugin.rules);
+ for (const [ruleName, rule] of rules) {
+ logger.logUpdate(logger.colors.yellow(` Generating > ${ruleName}`));
+
+ const ruleFile = new RuleFile(plugin, pluginDirectory, ruleName, rule);


I'm always unsure about the typedef rule 😕
Here is a perfect example:

const rules = Object.entries(plugin.rules) You cannot tell what type rules has without checking in IDE, but I'm in GitHub WebView while reviewing the PR.

On the other side const ruleFile = new RuleFile(plugin, pluginDirectory, ruleName, rule), it's of type RuleFile, obviously, so I wont require a typedef here.

Because of the first example, I tend to enforce it just for everything. But I would love to configure it somehow to only enforce it, when it's not obviously 🤔

If we have a solution for this, please lets do this in another PR 🙂

Okay I know what you mean
However, wouldn't it be easier to do your review in VScode? with github CLI it's super convenient, in two seconds you can checkout the branch

The thing is, even adding the type annotations only on this kind of lines, reduces the readability a lot I think.

For each variable declaration, we'll add the double of code. And every other time we'll go over the printWidth limit of prettier, so it will be split into two lines. This really makes the code less readable, and "polluted". And when we develop, in our IDE, we don't need these explicit annotations.

I don't want to impose myself and change the workflow you're used to, so I'm sorry, but I think we'd gain a lot in readability 😅

tsconfig.json

fisker · 2022-10-04T10:42:39Z

scripts/generate-rule-files/cli.ts

+ .slice(2)
+ .find((arg) => arg.startsWith('--plugins='))
+ ?.replace('--plugins=', '')
+ .split(',');


We can use util.parseArgs https://nodejs.org/api/util.html#utilparseargsconfig

Yes we could but it is only with node 18. I didn't want to change the version of node used. So why not, but under control of @Shinigami92

Lets do such optimisations in later PRs

fisker · 2022-10-04T10:43:37Z

scripts/generate-rule-files/cli.ts

+ ?.replace('--plugins=', '')
+ .split(',');
+
+run({ plugins: selectedPlugins }).catch((err) => console.error(err));


Can we use top level await?

Would require to move to ESM. I don't think it's worth the hassle right now

But if you guys think it's necessary, I can take a look !

For now the project is not written in ESM, so lets do this in another PR when needed

Shinigami92 · 2022-10-04T14:48:26Z

I don't get any errors on my side when executing 🤔 is this correct?

Screen.Recording.2022-10-04.at.16.44.02.mov

Beside that, there are some things that are still not that well like:

and

I would also love if it shows: new generated, updated and errored counts

For now there is nothing blocking anymore for this PR, so I can merge it now 🥳
But I would love to have you on board and enhance it further with more PRs 🙂

Julien-R44 · 2022-10-04T14:55:33Z

I don't get any errors on my side when executing 🤔 is this correct?

Yes, that is correct 😁. In fact, the errors on the old script came from 'json-schema-to-typescript'. So the plugin generate the types, then format it with prettier.

The problem is that sometimes it generated syntactically incorrect files. In particular, comments that sometimes contained "*/" (because this was in the rule description).

This generated syntactically incorrect files, and prettier crashed on this.

So what I did was to disable the plugin formatting as you can see here:
https://github.com/JuliensForks/eslint-define-config/blob/refactor/generation/scripts/generate-rule-files/src/json-schema-to-ts.ts#L28

Then remove this character from the rule description that was causing problems here :
https://github.com/JuliensForks/eslint-define-config/blob/refactor/generation/scripts/generate-rule-files/src/rule-file.ts#L64

So we even have new rules that were missed because of this problem !

And yes, it would be a great pleasure to continue to improve the project with future PRs!

Thanks again 💖

wip

6b0c727

Shinigami92 requested changes Oct 3, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

Shinigami92 assigned Julien-R44 Oct 3, 2022

Shinigami92 added the enhancement New feature or request label Oct 3, 2022

Shinigami92 reviewed Oct 3, 2022

View reviewed changes

scripts/generate-rule-files/index.ts Outdated Show resolved Hide resolved

Julien-R44 added 3 commits October 3, 2022 19:24

refactor: use node import protocol

5580f7f

refactor: remove old script

48d7feb

chore: change semver matcher

244f916

Shinigami92 reviewed Oct 3, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

Julien-R44 and others added 6 commits October 3, 2022 20:08

Update package.json

10a6d18

Co-authored-by: Shinigami <[email protected]>

final fixes + basic tests

f342d6b

fix: jsdoc rules prefix

45d66cc

chore: update lockfile

633cd00

Merge branch 'main' into refactor/generation

704e63f

Merge branch 'main' into refactor/generation

1c43b2b

Julien-R44 marked this pull request as ready for review October 3, 2022 21:28

chore: fix typing errors

f0cab6a

Shinigami92 reviewed Oct 4, 2022

View reviewed changes

Julien-R44 added 2 commits October 4, 2022 12:28

chore: update @poppinss/cliui and remove override

cef8f28

refactor: remove @poppinss/dev-utils since no longer used

745816b

fisker reviewed Oct 4, 2022

View reviewed changes

Shinigami92 approved these changes Oct 4, 2022

View reviewed changes

Shinigami92 merged commit b765cd3 into eslint-types:main Oct 4, 2022

Shinigami92 linked an issue Oct 4, 2022 that may be closed by this pull request

Refactoring generation script #134

Closed

Julien-R44 deleted the refactor/generation branch April 26, 2023 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring generation script #135

Refactoring generation script #135

Julien-R44 commented Oct 3, 2022

Shinigami92 commented Oct 3, 2022

Shinigami92 left a comment

Julien-R44 commented Oct 3, 2022

Julien-R44 commented Oct 3, 2022 •

edited

Loading

Julien-R44 commented Oct 3, 2022 •

edited

Loading

Shinigami92 Oct 4, 2022

Julien-R44 Oct 4, 2022

fisker Oct 4, 2022

Julien-R44 Oct 4, 2022

Shinigami92 Oct 4, 2022

fisker Oct 4, 2022

Julien-R44 Oct 4, 2022

Shinigami92 Oct 4, 2022

Shinigami92 commented Oct 4, 2022

Julien-R44 commented Oct 4, 2022 •

edited

Loading

Refactoring generation script #135

Refactoring generation script #135

Conversation

Julien-R44 commented Oct 3, 2022

Shinigami92 commented Oct 3, 2022

Shinigami92 left a comment

Choose a reason for hiding this comment

Julien-R44 commented Oct 3, 2022

Julien-R44 commented Oct 3, 2022 • edited Loading

Julien-R44 commented Oct 3, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shinigami92 commented Oct 4, 2022

Julien-R44 commented Oct 4, 2022 • edited Loading

Julien-R44 commented Oct 3, 2022 •

edited

Loading

Julien-R44 commented Oct 3, 2022 •

edited

Loading

Julien-R44 commented Oct 4, 2022 •

edited

Loading