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

UnorderedKey fix #261

Merged
merged 2 commits into from Sep 23, 2020

Conversation

evgeniy-r
Copy link
Contributor

@evgeniy-r evgeniy-r commented Aug 14, 2020

PR for #252.

✔ Checklist:

  • This PR has been added to CHANGELOG.md (at the top of the list);
  • Tests for the changes have been added (for bug fixes / features);
  • Docs have been added / updated (for bug fixes / features).

@evgeniy-r evgeniy-r marked this pull request as ready for review August 16, 2020 08:11
@baile320
Copy link
Contributor

baile320 commented Aug 16, 2020

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.

@evgeniy-r
Copy link
Contributor Author

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.

@evgeniy-r
Copy link
Contributor Author

evgeniy-r commented Aug 16, 2020

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

@mgrachev mgrachev linked an issue Aug 19, 2020 that may be closed by this pull request
@mgrachev
Copy link
Member

@evgeniy-r Thank you for your contribution! 🔥

I need more time to review this PR 🙂

@DDtKey DDtKey mentioned this pull request Aug 21, 2020
3 tasks
@mgrachev mgrachev requested a review from a team August 21, 2020 13:33
@mgrachev mgrachev added this to the v2.2.0 milestone Aug 21, 2020
Copy link
Member

@DDtKey DDtKey left a comment

Choose a reason for hiding this comment

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

You did a great job and thank you for that! 👍

I left some related comments, please take a look.

src/fixes/unordered_key.rs Outdated Show resolved Hide resolved
src/fixes/unordered_key.rs Outdated Show resolved Hide resolved
src/fixes/unordered_key.rs Outdated Show resolved Hide resolved
@DDtKey DDtKey requested a review from a team August 27, 2020 19:53
@DDtKey
Copy link
Member

DDtKey commented Aug 27, 2020

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 ?

@mgrachev
Copy link
Member

mgrachev commented Aug 29, 2020

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 🤔

@DDtKey
Copy link
Member

DDtKey commented Aug 29, 2020

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.

@mgrachev mgrachev closed this Aug 29, 2020
@mgrachev mgrachev reopened this Aug 29, 2020
@mgrachev
Copy link
Member

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 🙏

@evgeniy-r
Copy link
Contributor Author

I was away, will answer all comments soon.

@evgeniy-r
Copy link
Contributor Author

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

@DDtKey
Copy link
Member

DDtKey commented Aug 31, 2020

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?

src/fixes/unordered_key.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

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

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/fixes/unordered_key.rs 99.28% <99.28%> (ø)
src/fixes.rs 95.08% <100.00%> (+0.04%) ⬆️
src/main.rs 80.43% <0.00%> (-10.87%) ⬇️

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.

@DDtKey DDtKey requested a review from a team August 31, 2020 20:59
@DDtKey
Copy link
Member

DDtKey commented Sep 1, 2020

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 🙂

@mgrachev
Copy link
Member

mgrachev commented Sep 2, 2020

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.

@evgeniy-r
Copy link
Contributor Author

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)? 🙂

@mgrachev
Copy link
Member

mgrachev commented Sep 5, 2020

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

Yes, the UnorderedKey check works as expected.

@evgeniy-r
Copy link
Contributor Author

Yes, the UnorderedKey check works as expected.

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

@evgeniy-r
Copy link
Contributor Author

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.

@evgeniy-r evgeniy-r force-pushed the feat/252/unordered_key_fixer branch 2 times, most recently from 18a92a3 to c44a16b Compare September 11, 2020 18:24
@evgeniy-r
Copy link
Contributor Author

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

src/fixes/unordered_key.rs Outdated Show resolved Hide resolved
src/fixes/unordered_key.rs Outdated Show resolved Hide resolved
src/fixes/unordered_key.rs Outdated Show resolved Hide resolved
src/fixes/unordered_key.rs Outdated Show resolved Hide resolved
src/fixes/unordered_key.rs Outdated Show resolved Hide resolved
src/fixes/unordered_key.rs Outdated Show resolved Hide resolved
tests/common/mod.rs Outdated Show resolved Hide resolved
tests/fixes/mod.rs Outdated Show resolved Hide resolved
tests/fixes/mod.rs Outdated Show resolved Hide resolved
@mgrachev mgrachev merged commit 36b904e into dotenv-linter:master Sep 23, 2020
@evgeniy-r evgeniy-r deleted the feat/252/unordered_key_fixer branch September 26, 2020 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add fixer: UnorderedKeyFixer
5 participants