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

Enable Perfectionist Eslint Rules #1403

Closed
wants to merge 5 commits into from

Conversation

MarvNC
Copy link
Member

@MarvNC MarvNC commented Sep 9, 2024

Resolves #1401
Perfectionist documentation
Current rules:

"perfectionist/sort-array-includes": ["error"],
"perfectionist/sort-astro-attributes": ["off"],
"perfectionist/sort-classes": ["off"],
"perfectionist/sort-enums": ["error"],
"perfectionist/sort-exports": ["error"],
"perfectionist/sort-imports": ["off"],
"perfectionist/sort-interfaces": ["error"],
"perfectionist/sort-intersection-types": ["error"],
"perfectionist/sort-jsx-props": ["off"],
"perfectionist/sort-maps": ["error"],
"perfectionist/sort-named-exports": ["error"],
"perfectionist/sort-named-imports": ["off"],
"perfectionist/sort-object-types": ["error"],
"perfectionist/sort-objects": ["error"],
"perfectionist/sort-sets": ["error"],
"perfectionist/sort-svelte-attributes": ["off"],
"perfectionist/sort-switch-case": ["error"],
"perfectionist/sort-union-types": ["error"],
"perfectionist/sort-variable-declarations": ["off"],
"perfectionist/sort-vue-attributes": ["off"]

@MarvNC
Copy link
Member Author

MarvNC commented Sep 9, 2024

I checked through all the changes.

Some places where there might have previously been semantic ordering:

Images

image
image
image
image
image

I think these are minor cases where the rule could be disabled on a case-by-case basis when someone feels strongly about it, but I feel they are fine alphabetical.

One test failed due to these changes and I had to change the ordering of these two in the array to fix it:
https://github.com/themoeway/yomitan/blob/cd431c2c566f1e4809655d5fd3c91e0b31ae0ccc/test/data/translator-test-results.json#L19898-L19901

I tested it and this is because in this file objects were not previously sorted and the test reflected this, so it shouldn't cause issues.
https://github.com/themoeway/yomitan/blob/cd431c2c566f1e4809655d5fd3c91e0b31ae0ccc/ext/js/language/language-descriptors.js#L212-L220

You can verify this by changing the order of preprocessors in this file and seeing if the tests fail.

@MarvNC MarvNC marked this pull request as ready for review September 9, 2024 04:36
@MarvNC MarvNC requested a review from a team as a code owner September 9, 2024 04:36
@MarvNC
Copy link
Member Author

MarvNC commented Sep 9, 2024

I was wrong, it appears to be semantic. Remade changes with that part eslint ignored, will refactor to array later on.

NVM on the refactoring

@MarvNC MarvNC force-pushed the enable-perfectionist branch from cd431c2 to 8ab1fbc Compare September 9, 2024 05:20
@jamesmaa
Copy link
Collaborator

jamesmaa commented Sep 9, 2024

I feel like from the random spotchecking I'm seeing there are a lot examples in which I feel like sorting is making the code less readable rather than more.

It feels like trying to sort alphabetically is less often intended than not intended. A lot of the ordering of keys, enums, maps are done by grouping semantics and doing anything else impacts readability in a negative way

@MarvNC MarvNC closed this Sep 17, 2024
@MarvNC MarvNC deleted the enable-perfectionist branch September 17, 2024 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Relevant Perfectionist Rules
2 participants