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
UnorderedKey fix #261
Conversation
67635e8
to
47e9a33
Compare
Quick question: should this support key sorting within groupings? For example:
after running the fixer, the file could look like:
When I run the current fixer, I actually get:
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 ( 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 🙂 |
There was a problem hiding this 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.
Among other things, I noticed that fixer works exclusively with I understand that the warning One option is of course to check the control comments in the fixer. But this will create duplicate logic. 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 ? |
Good catch! You're absolutely right.
Can you give more information about the problem with
No, I don't think so. We should find a solution before merging this PR 🤔 |
Sorry, I was wrong (typo after GH suggest), I meant
I fully agree. |
Let me know. Thank you 🙏 |
I was away, will answer all comments soon. |
1bfe0b3
to
5d69ae6
Compare
Before discussing control comments, I have a question. File:
Linter's output:
|
I think there is some logic in this. From my point of view, this is the expected result, and in this case, the following option looks correct for separation:
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? |
e064167
to
0223e0c
Compare
Codecov Report
@@ 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.
|
I created an issue #278 with examples 🙂 |
I think we should remove the logic for displaying |
So, the answer is "yes" (that is the desired behavior)? 🙂 |
Yes, the |
I plan to submit a PR tomorrow (control comments). |
I found an interesting kind of issues: Before fixing the order:
After fixing:
And we will get the incorrect file. Therefore, I believe that we should split the groups by control comments, as well as by blank lines. I made appropriate changes. |
18a92a3
to
c44a16b
Compare
I moved changes in checker to the separate PR: #281 |
b6efd03
to
967be02
Compare
4e300fe
to
2f3becb
Compare
2f3becb
to
b8f1e1f
Compare
PR for #252.
✔ Checklist: