Skip to content

Conversation

@hugop95
Copy link
Contributor

@hugop95 hugop95 commented Aug 27, 2024

Description

Resolves (partially) #214. The sort-classes rule is not part of this PR as checking if a group entered by the user is part of predefined groups requires a more complex strategy which I believe deserves its own PR.

Checks and validates group configuration for rules using groups. Configurations referencing groups that are

  • not part of predefined groups
  • and not defined in customGroups

are very likely user mistakes or typos. These groups are considered invalid and will throw an error, stopping ESLint.

image

Affected rules:

  • sort-astro-attributes
  • sort-imports
  • sort-interfaces
  • sort-intersection-types
  • sort-jsx-props
  • sort-object-types
  • sort-object
  • sort-svelte-attributes
  • sort-union-types
  • sort-vue-attributes

Remaining to do:

  • Add tests for each rule affected.

Additional context

  • Some tests have been slightly changed as they were referencing invalid groups. In those cases, invalid groups were removed from the test configuration.

  • Ideally, we should be testing that invalid group configurations throw an error for each rule. Unfortunately, it is currently not possible to test rules that throw errors (Add ability to test rule schemas eslint/eslint#13434 | feat: ability to test rule errors and invalid schemas eslint/eslint#16823). Furthermore, getting 100% code coverage in validateGroupsConfiguration.ts is not possible only through ESLint's RuleTester as there is no way to catch
    the error thrown. We get 100% coverage temporarily in that file with a simple unit test in validateGroupsConfiguration.test.ts until that feature is implemented in ESLint.

What is the purpose of this pull request?

  • New Feature

@hugop95 hugop95 marked this pull request as ready for review August 28, 2024 10:15
@hugop95 hugop95 force-pushed the feat/all-rules/#214 branch from eeb9c95 to 6c7b5c2 Compare August 28, 2024 21:17
@hugop95 hugop95 force-pushed the feat/all-rules/#214 branch from 6c7b5c2 to 1f8c50a Compare August 31, 2024 20:21
Copy link
Owner

@azat-io azat-io left a comment

Choose a reason for hiding this comment

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

Nice job! ❤️

{
...options,
customGroups: { top: ['c', 'b'] },
groups: ['top', 'unknown'],
Copy link
Owner

Choose a reason for hiding this comment

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

Why remove 'unknown'?

Copy link
Contributor Author

@hugop95 hugop95 Sep 1, 2024

Choose a reason for hiding this comment

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

Fixed. Mistake on my part.

import { validateGroupsConfiguration } from '../utils/validate-groups-configuration'

/**
* It is currently not possible to test rules that throw errors (https://github.com/eslint/eslint/issues/13434),
Copy link
Owner

Choose a reason for hiding this comment

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

Let's try to stick to the 80 character line length limit rule as stated in the prettier config?
I suggest breaking the comment down into a few lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@hugop95 hugop95 force-pushed the feat/all-rules/#214 branch from 1f8c50a to 8aad99e Compare September 1, 2024 14:24
@hugop95 hugop95 force-pushed the feat/all-rules/#214 branch from 8aad99e to 2009810 Compare September 1, 2024 15:43
@azat-io azat-io merged commit 0872bdd into azat-io:main Sep 1, 2024
@azat-io
Copy link
Owner

azat-io commented Sep 1, 2024

Great. Thank you!

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.

2 participants