Feature: display the common group between two imports in the error message

[hugop95]

What rule do you want to change?

sort-imports (but can be applied to other rules that handle group as well)

Describe the problem

sort-imports's grouping behavior can be a bit confusing in some cases (see the latest issue #470), and it's not easy for users to see if an import actually belongs to the group they think they should.

Two imports from the same group that are misplaced will return the following error:
Expected "NEXT IMPORT" to come before "PREVIOUS IMPORT".

I suggest that we improve this error message to show the group both imports belong to:

Expected "NEXT_IMPORT" to come before "PREVIOUS_IMPORT" (both in group "COMMON_GROUP").

Additional details

• When groups is [] or [unknown], the common group will always be unknown, so we should keep a simple error message in that case.

Validations

• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.

[hugop95]

Hi @azat-io 👋

Let me know what you think of this idea, a lot of tests would need to be updated (the error message returned wouldn't be the same anymore), so I want to have your opinion before starting any implementation 🙂

[azat-io]

I don't have a clear opinion on that. On the one hand, maybe it's convenient. On the other hand it may be unnecessary information and too long error messages.

I would like to hear other opinions.

[hugop95]

Mandatory @OlivierZal ping 🙂

[OlivierZal]

From a user point of vue, I would rather be in favour of more explicit messages, but I know it's a lot to change, so is the balance code cost / user complaints worth it? In particular, the 2 import issues recently raised were about the activation of the sideEffect option, not directly about the group type (even if it's related), weren't they?

[hugop95]

@OlivierZal

but I know it's a lot to change, so is the balance code cost / user complaints worth it?

From a code perspective, it's actually very straightforward and doesn't require a lot of code, but this will require most of the tests expected results to be updated. Mostly chore stuff 😄

the 2 import issues recently raised were about the activation of the sideEffect option, not directly about the group type (even if it's related), weren't they?

Those issues were about side effects indeed. Here, it's more about users not being aware the side effect sorting is disabled and that an option exists for that.

[OlivierZal]

To sum up: long but easy work, can give added value to users.

[hugop95]

@OlivierZal

To sum up: long but easy work, can give added value to users.

Pretty much!

I think that the usefulness is more pronounced in rules where grouping might not behave like users would think, such as sort-imports or sort-classes, especially since it's common for users to also add their own additional groups through customGroups. Enabling this feature only for those two rules would probably be the best approach for now.

[OlivierZal]

I don't think it's a must have, but surely a nice to have.

[hugop95]

Hi @azat-io 👋

Rechecking on this issue since it's been a while. What would you think today?

I believe that it really helps debugging complex configurations (typically in sort-imports).

[azat-io]

I don't mind if it makes things clearer, let's do it. 👍

But personally, I don't see any benefit in it yet.

[hugop95]

I underestimated the changes required a bit, especially regarding this part:

When groups is [] or [unknown], the common group will always be unknown, so we should keep a simple error message in that case.

On top of having to update all tests. I'm keeping this in the backlog for now.