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: adds customGroup.newlinesInside option #428

Merged
merged 13 commits into from
Dec 23, 2024

Conversation

hugop95
Copy link
Contributor

@hugop95 hugop95 commented Dec 21, 2024

First PR aiming to partially resolve the issue #401.

  • ⚙️ PR 1 (this): Add customGroup.newlinesInside option. We will take the opportunity to create a getNewlinesBetweenOption method that will help us later to precisely customize newline behavior between two specific groups.
  • PR 2: Add a way to choose a specific newline behavior between two groups. See the proposals here.

Description

We currently have the newlinesBetween option, which applies the same newlines-related rules between all groups.
This PR adds a sub-option for the new array-based customGroups:

  • newlinesInside?: 'always' | 'never'

Here is the list of rules with this new addition:

  • sort-classes
  • sort-modules
  • sort-object-types
  • sort-interfaces

New option

customGroup.newlinesInside?: 'ignore' | 'always' | 'never'

Newlines rule that should be applied between two consecutive nodes of the same custom group.

Example: Enforce a newline between all class methods.

{
  customGroups: [
    {
      groupName: "methodsWithNewlinesBetween",
      selector: "method",
      newlinesInside: "always"
    }
  ]
}

Other changes

  • Unifies the way spacing errors are reported:
    • The leftNum < rightNum check is currently only made if numberOfEmptyLinesBetween === 0 (as confirmed by passing tests).
    • With newlinesBetween: always and if numberOfEmptyLinesBetween > 1, we were not checking if leftNum < rightNum.
    • The leftNum < rightNum check is not made when newlinesBetween: never either (as confirmed by passing tests).
    • => Always check if leftNum < rightNum.
    • The few tests impacted can be found in those commits: 04132ee, 252873c.
      • Only some spacing errors reported were removed, outputs are unchanged.

What is the purpose of this pull request?

  • New Feature

@hugop95 hugop95 force-pushed the feat/newlines-between branch 10 times, most recently from 25f887d to ead3cc3 Compare December 21, 2024 23:53
@hugop95 hugop95 changed the title feat(sort-classes): customGroup.newlinesBetweenrelated options feat: adds customGroup.newlinesBetweenrelated options Dec 21, 2024
@hugop95 hugop95 force-pushed the feat/newlines-between branch 4 times, most recently from 7c6eb6c to a1c3d55 Compare December 22, 2024 19:02
@hugop95 hugop95 force-pushed the feat/newlines-between branch 2 times, most recently from bd17725 to 36f886b Compare December 22, 2024 19:21
@hugop95 hugop95 changed the title feat: adds customGroup.newlinesBetweenrelated options feat: adds customGroup. newlinesInside option Dec 22, 2024
@hugop95 hugop95 changed the title feat: adds customGroup. newlinesInside option feat: adds customGroup.newlinesInside option Dec 22, 2024
@hugop95 hugop95 force-pushed the feat/newlines-between branch from 36f886b to dde07dd Compare December 22, 2024 19:35
- We are going to be adding the option in the next commit.
- We will be adding auto-fix for this reported error in a commit later.
…nodes

- The `leftNum < rightNum` check is currently only made if `numberOfEmptyLinesBetween === 0` (as confirmed by passing tests).
- With `newlinesBetween: always` and if `numberOfEmptyLinesBetween > 1`, we were not checking whether the two nodes were sorted or not before raising the spacing error.
- This would result in two errors toward the same two nodes:
  - One related to invalid order.
  - The other related to invalid spacing.
- A few tests had to be updated.
  - All of those errors targeted nodes that already had order errors reported for them.
…odes

- The `leftNum < rightNum` check is not made when `newlinesBetween: never` either (as confirmed by passing tests).
- A few tests had to be updated:
  - The fix itself has no change. There are only slightly less reported newlines errors.
  - All of those errors targeted nodes that already had order errors reported for them.
- Renames constants.
- Simplifies `for` loop.
- Creates constants.
@hugop95 hugop95 force-pushed the feat/newlines-between branch from dde07dd to afaa54f Compare December 22, 2024 19:41
@hugop95 hugop95 force-pushed the feat/newlines-between branch from afaa54f to cb1f483 Compare December 22, 2024 19:47
@hugop95 hugop95 marked this pull request as ready for review December 22, 2024 19:51
@hugop95
Copy link
Contributor Author

hugop95 commented Dec 22, 2024

@OlivierZal 👋 Feel free to review this if you want 🙂

@@ -0,0 +1,205 @@
import { describe, expect, it } from 'vitest'
Copy link
Owner

Choose a reason for hiding this comment

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

Can you move this file to test/utils/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@azat-io Oops, rebase from #425 didn't move it 😅. Moved in f4b4ad5.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

@@ -290,13 +290,6 @@ describe(ruleName, () => {
},
messageId: 'unexpectedImportsGroupOrder',
},
{
Copy link
Owner

Choose a reason for hiding this comment

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

Can you tell me why you removed these tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@azat-io

  • The edited code responsible for those lines changes can be found here (see each commit's description)
  • Those changes are not absolutely required:
    • (1) Some spacing errors were reported after checking if leftNum < rightNum.
    • (2) Some other errors were not checking if leftNum < rightNum, leading to spacing errors being reported even when the nodes were not sorted.
    • I added the leftNum < rightNum for all cases => Spacing errors from (2) are now not reported.
  • If needed, I can revert these commits. 🙂

@azat-io azat-io merged commit 3b3d2d5 into azat-io:main Dec 23, 2024
8 checks passed
@hugop95 hugop95 deleted the feat/newlines-between branch December 23, 2024 23:21
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.

3 participants