-
-
Notifications
You must be signed in to change notification settings - Fork 26
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
Refactoring generation script #135
Refactoring generation script #135
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Glad you like it man! 🎉🎉 |
Okay so I had to do a temporary fix. We had some type errors on So I overrided this dependency in the package.json, field 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 ! |
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅
.slice(2) | ||
.find((arg) => arg.startsWith('--plugins=')) | ||
?.replace('--plugins=', '') | ||
.split(','); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use util.parseArgs
https://nodejs.org/api/util.html#utilparseargsconfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets do such optimisations in later PRs
?.replace('--plugins=', '') | ||
.split(','); | ||
|
||
run({ plugins: selectedPlugins }).catch((err) => console.error(err)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use top level await?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now the project is not written in ESM, so lets do this in another PR when needed
I don't get any errors on my side when executing 🤔 is this correct? Screen.Recording.2022-10-04.at.16.44.02.movBeside 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 🥳 |
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: Then remove this character from the rule description that was causing problems here : 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 💖 |
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
pnpm generate:rules --plugins=vue-pug,node,eslint
( 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!