Skip to content
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

feat: types with intellisense (#152) #177

Closed
wants to merge 13 commits into from

Conversation

Dimava
Copy link
Contributor

@Dimava Dimava commented Mar 22, 2023

  • Rewrite rules from Rule<ARuleOptions> to inlined Rule<[RuleLevel, 'option1' | 'option2']> to make understandable hover intellisence in defineConfig
  • Rewrite rules from interface to type to shorten (property) ARule['a-rule']: Rule<[RuleLevel]> to (property) 'a-rule': Rule<[RuleLevel]>
  • Remove unused calls and types
  • Make sure all rules compile

@Dimava Dimava force-pushed the intellisense branch 2 times, most recently from 8ca6fe7 to e05f429 Compare March 22, 2023 22:07
@Dimava
Copy link
Contributor Author

Dimava commented Mar 22, 2023

Variant 1:
image
Variant 2:
image

@Dimava Dimava marked this pull request as ready for review March 22, 2023 22:10
@Dimava Dimava requested a review from Shinigami92 as a code owner March 22, 2023 22:10
@Dimava
Copy link
Contributor Author

Dimava commented Mar 22, 2023

(PR requires a rebuild and a cleanup, reqiring a review on if it looks properly at all)

@Dimava
Copy link
Contributor Author

Dimava commented Mar 22, 2023

@Julien-R44 ref

@Dimava
Copy link
Contributor Author

Dimava commented Mar 22, 2023

🤡 Variant 3 with Latin capital letter glottal stop: Ɂ
image

@Shinigami92
Copy link
Collaborator

Running pnpm run generate:rules results in some errors (tested on my Mac M1)

image

Additionally checking the diff after the generation

image

This does not look right to me

  1. It should take AccessorPairsRuleConfig (RuleConfig), because the whole type/interface chain are build for that from top to bottom in each rule file.
  2. I'm not sure if it is necessary to have never, never in each RuleConfigExt. If really needed, these should be defaults.

@Dimava Dimava force-pushed the intellisense branch 2 times, most recently from 0a183db to e1c018d Compare March 24, 2023 19:14
Copy link
Collaborator

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vitest type-check tests are failing, please have a look into them

src/rules/rule-config.d.ts Outdated Show resolved Hide resolved
src/rules/experimental.d.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 marked this pull request as draft March 29, 2023 08:16
@Dimava
Copy link
Contributor Author

Dimava commented Mar 31, 2023

  • cleanup leftovers
  • manually fix leftover broken rules
  • nr generate:rules && nr format && nr lint && nr typecheck ok
  • regenerate test snapshot, nr test ok
    (nr is alias for pnpm run)

@Dimava Dimava marked this pull request as ready for review March 31, 2023 22:03
@Dimava Dimava requested a review from Shinigami92 April 1, 2023 10:07
Copy link
Collaborator

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not check every rule yet, but when I run pnpm run generate:rules the result is looking promising 🙂
So good work for now 👍

However, I'm tempted to split this PR into several commits to make it more discoverable in the commit history

I would like to split the "Apply handcrafted hard to generalize fixes" into its own PR and also add a terminal log to notify everytime a rule was "patched"

@Shinigami92 Shinigami92 added the enhancement New feature or request label Apr 2, 2023
@Shinigami92
Copy link
Collaborator

This now needs to be rebased

@Dimava Dimava force-pushed the intellisense branch 2 times, most recently from 8024f43 to 6718d3b Compare April 4, 2023 00:15
package.json Outdated Show resolved Hide resolved
@Dimava
Copy link
Contributor Author

Dimava commented Apr 4, 2023

I thing I could really use a diff-generator script

I have like 10 rules to fix, doing that by hand is becoming hard

@Shinigami92
Copy link
Collaborator

In that case we should use the package rimraf

@Dimava Dimava requested a review from Shinigami92 April 4, 2023 16:22
@Shinigami92
Copy link
Collaborator

@Dimava I have checked and tried to add such a preflight script, but deleting src/rules would also result in deleting other files that wont get regenerated
So all in all it might be a bad idea and we should just leave as is
The files will be automatically overridden when they get generated anyways
A human should always check the diff before pushing to git always (as I already did for everytime I generate rules)

@Shinigami92
Copy link
Collaborator

@Dimava I checked the PR freshly and run pnpm run generate:rules
Then I looked into .eslintrc.js and saw a bunch of errors
These need to be fixed!

e.g. RuleLevel should also be a singular config option if the other options are optional

Beside that, I'm not quite sure what you mean by

Rewrite rules from Rule to inlined Rule<[RuleLevel, 'option1' | 'option2']> to make understandable hover intellisence in defineConfig
Rewrite rules from interface to type to shorten (property) ARule['a-rule']: Rule<[RuleLevel]> to (property) 'a-rule': Rule<[RuleLevel]>

Right now the diff of the PR is really huge and I'm not sure if there is even a difference between type and interface in these cases.
Please provide me a screenshot what would be different by using type over interface

@Dimava
Copy link
Contributor Author

Dimava commented Apr 4, 2023

@Dimava I have checked and tried to add such a preflight script, but deleting src/rules would also result in deleting other files that wont get regenerated

That's why I deleted srcr/rules/*/

@Shinigami92
Copy link
Collaborator

@Dimava please also answer my second question as well

Check the branch and run pnpm run generate:rules and you will see that far more than 100 rules are updated
So what is the benefit out of that?


Copy of my question above:

Beside that, I'm not quite sure what you mean by

Rewrite rules from Rule to inlined Rule<[RuleLevel, 'option1' | 'option2']> to make understandable hover intellisence in defineConfig
Rewrite rules from interface to type to shorten (property) ARule['a-rule']: Rule<[RuleLevel]> to (property) 'a-rule': Rule<[RuleLevel]>

Right now the diff of the PR is really huge and I'm not sure if there is even a difference between type and interface in these cases.
Please provide me a screenshot what would be different by using type over interface

@Dimava Dimava mentioned this pull request Apr 18, 2023
@Shinigami92
Copy link
Collaborator

stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants