Quick question: should this support key sorting within groupings?

For example:

# Grouping 1
A=B
C=B
B=D

# Grouping 2
THIS=THAT
Z=X
Q=R

after running the fixer, the file could look like:

# Grouping 1
A=B
B=D
C=B

# Grouping 2
Q=R
THIS=THAT
Z=X

When I run the current fixer, I actually get:

# Grouping 1
A=B
B=D
C=B

Q=R
# Grouping 2
THIS=THAT
Z=X

which is not behavior I would expect if I were a user because I think users frequently group common functionality together in env files (all database connections go near each other, all message broker connections near each other, etc). If it is decided to keep the functionality as it currently is, it might be helpful to add explanations in the doc somewhere. Currently, the docs say "You can use blank lines to split lines into groups" (https://github.com/dotenv-linter/dotenv-linter#unordered-key) so that might lead to confusion.

Open to hearing other people's thoughts though.

This behavior is due to the fact that we can handle comments in different ways.

I assumed that the comment is most likely related to the next line with a key (# Grouping 2 is related to THIS=THAT).
So it is handled as a whole.

It can be disscussed and changed if necessary.

And yes, it sorts the keys separately in each group.

@evgeniy-r Thank you for your contribution! 🔥

I need more time to review this PR 🙂

Among other things, I noticed that fixer works exclusively with lines (without checking warnings!) and always sort full file.
But we have control comments(#237) and not the entire file should be affected by this Fixer, but only really existing Warnings.

I understand that the warning line_number can break in fix process, but this also does not seem to be a suitable solution.
And it's really trouble...
I also found this in the recently added EndingBlankLineFixer (#263)

One option is of course to check the control comments in the fixer. But this will create duplicate logic.
At the same time, changing line_number in Warnings is also not correct, because when displaying results, we must point to the line of error in the original file (but when there are implementations of all fixers, we may not print this at all).

Perhaps this is beyond the scope of this task at the moment, but we cannot forget about it.

What do you think @dotenv-linter/core @evgeniy-r ?

Among other things, I noticed that fixer works exclusively with lines (without checking warnings!) and always sort full file.
But we have control comments(#237) and not the entire file should be affected by this Fixer, but only really existing Warnings.

Good catch! You're absolutely right.
All checks and fixes should be disabled using controll comments or the --skip argument.

I understand that the warning line_number can break in fix process, but this also does not seem to be a suitable solution.
And it's really trouble...
I also found this in the recently added EndingBlankLineFixer (#263)

Can you give more information about the problem with EndingBlankLineFixer and controll comments?
I've just checked it and it's worked with the control comment # dotenv-linter:off EndingBlankLine.
Or you can open an issue and we will discuss about it.

One option is of course to check the control comments in the fixer. But this will create duplicate logic.
At the same time, changing line_number in Warnings is also not correct, because when displaying results, we must point to the line of error in the original file (but when there are implementations of all fixers, we may not print this at all).

Perhaps this is beyond the scope of this task at the moment, but we cannot forget about it.

No, I don't think so. We should find a solution before merging this PR 🤔

Can you give more information about the problem with EndingBlankLineFixer and controll comments?
I've just checked it and it's worked with the controll comment # dotenv-linter:off EndingBlankLine.
Or you can open an issue and we will discuss about it.

Sorry, I was wrong (typo after GH suggest), I meant ExtraBlankLineFixer (#260) and this review comment
A little later, I can double-check and create an issue.

No, I don't think so. We should find a solution before merging this PR thinking

I fully agree.

Sorry, I was wrong (typo after GH suggest), I meant ExtraBlankLineFixer (#260) and this review comment
A little later, I can double-check and create an issue.

Let me know. Thank you 🙏

I was away, will answer all comments soon.

Before discussing control comments, I have a question.
Is this the expected behavior?

File:

X=Y
# dotenv-linter:off UnorderedKey
A1=B
A2=B

# dotenv-linter:on UnorderedKey
A=B

Linter's output:

.env:7 UnorderedKey: The A key should go before the X key

Found 1 problem

Before discussing control comments, I have a question.
Is this the expected behavior?

I think there is some logic in this.
Actually, the group is separated in the comment block and it (block) is not surrounded by blank lines. But it's may be not clear.

From my point of view, this is the expected result, and in this case, the following option looks correct for separation:

X=Y
# dotenv-linter:off UnorderedKey
A1=B
A2=B
# dotenv-linter:on UnorderedKey

A=B

That is, I would not consider comments attached to specific lines if they are separated from them(key\value line) by a line break 🤔

Maybe @mgrachev has a different vision on this?

Codecov Report

Merging #261 into master will decrease coverage by 0.09%.
The diff coverage is 99.30%.

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
- Coverage   97.64%   97.54%   -0.10%     
==========================================
  Files          31       32       +1     
  Lines        2634     2775     +141     
==========================================
+ Hits         2572     2707     +135     
- Misses         62       68       +6     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a86e76...0223e0c. Read the comment docs.

Sorry, I was wrong (typo after GH suggest), I meant ExtraBlankLineFixer (#260) and this review comment
A little later, I can double-check and create an issue.

Let me know. Thank you

I created an issue #278 with examples 🙂

At the same time, changing line_number in Warnings is also not correct, because when displaying results, we must point to the line of error in the original file (but when there are implementations of all fixers, we may not print this at all).

I think we should remove the logic for displaying Fixed/Unfixed Warnings after we merge this PR.

I think there is some logic in this.
Actually, the group is separated in the comment block and it (block) is not surrounded by blank lines. But it's may be not clear.

So, the answer is "yes" (that is the desired behavior)? 🙂

So, the answer is "yes" (that is the desired behavior)? 🙂

Yes, the UnorderedKey check works as expected.

Yes, the UnorderedKey check works as expected.

I plan to submit a PR tomorrow (control comments).

I found an interesting kind of issues:

Before fixing the order:

BB=1
# dotenv-linter:off LowercaseKey
Aa=B
X=X
# dotenv-linter:on LowercaseKey

After fixing:

Aa=B
BB=1
# dotenv-linter:off LowercaseKey
X=X
# dotenv-linter:on LowercaseKey

And we will get the incorrect file.
If we fix the Aa key somehow, it will not be the best solution either, because a user doesn't want fix the lower case.

Therefore, I believe that we should split the groups by control comments, as well as by blank lines.

I made appropriate changes.

I moved changes in checker to the separate PR: #281