Repository: dotenv-linter/dotenv-linter
Pull request #261 (2020-08-14 17:57:23)

UnorderedKey fix
PR for https://github.com/dotenv-linter/dotenv-linter/issues/252.

#### ✔ Checklist:
<!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->

- [x] This PR has been added to [CHANGELOG.md](https://github.com/dotenv-linter/dotenv-linter/blob/master/CHANGELOG.md) (at the top of the list);
- [x] Tests for the changes have been added (for bug fixes / features);
- [x] Docs have been added / updated (for bug fixes / features).

<!-- _NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open._ -->


baile320: 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 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 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: 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: And yes, we sort the keys separately in each group.
mgrachev: @evgeniy-r Thank you for your contribution! 🔥

I need more time to review this PR 🙂
DDtKey: Among other things, I noticed that we work exclusively with `lines` (**without checking warnings!**) and we always sort them.
But we have control comments(#237) and not the entire file should be affected by this `Fixer`, but only really existing `Warning`s.

I understand that the order can break, 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)

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: > 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 `Warning`s.

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 controll 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 `Warning`s 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: > 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), I meant `ExtraBlankLineFixer` (#260) and [this review comment](https://github.com/dotenv-linter/dotenv-linter/pull/260#discussion_r471299204)
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: > Sorry, I was wrong (typo after GH suggest), I meant `ExtraBlankLineFixer` (#260) and [this review comment](https://github.com/dotenv-linter/dotenv-linter/pull/260#discussion_r471299204)
> A little later, I can double-check and create an issue.

Let me know. Thank you 🙏



evgeniy-r: I was away, will answer all comments soon.
evgeniy-r: 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: > 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.

But 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 :thinking: 

Maybe @mgrachev has a different vision on this?
codecov-commenter: # [Codecov](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/261?src=pr&el=h1) Report
> Merging [#261](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/261?src=pr&el=desc) into [master](https://codecov.io/gh/dotenv-linter/dotenv-linter/commit/5a86e7649f68d3f167cfdcab503f44e1ee5518b7?el=desc) will **decrease** coverage by `0.09%`.
> The diff coverage is `99.30%`.

[![Impacted file tree graph](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/261/graphs/tree.svg?width=650&height=150&src=pr&token=wQybePfHI1)](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/261?src=pr&el=tree)

```diff
@@            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](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/261?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/fixes/unordered\_key.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/261/diff?src=pr&el=tree#diff-c3JjL2ZpeGVzL3Vub3JkZXJlZF9rZXkucnM=) | `99.28% <99.28%> (ø)` | |
| [src/fixes.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/261/diff?src=pr&el=tree#diff-c3JjL2ZpeGVzLnJz) | `95.08% <100.00%> (+0.04%)` | :arrow_up: |
| [src/main.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/261/diff?src=pr&el=tree#diff-c3JjL21haW4ucnM=) | `80.43% <0.00%> (-10.87%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/261?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/261?src=pr&el=footer). Last update [5a86e76...0223e0c](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/261?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

DDtKey: > > Sorry, I was wrong (typo after GH suggest), I meant `ExtraBlankLineFixer` (#260) and [this review comment](https://github.com/dotenv-linter/dotenv-linter/pull/260#discussion_r471299204)
> > A little later, I can double-check and create an issue.
> 
> Let me know. Thank you

I created an issue #278 with examples :slightly_smiling_face: 
mgrachev: > At the same time, changing `line_number` in `Warning`s 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: > 
> 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)? :slightly_smiling_face: 

mgrachev: > So, the answer is "yes" (that is the desired behavior)? 🙂

Yes, the `UnorderedKey` check works as expected.


evgeniy-r: > Yes, the UnorderedKey check works as expected.

I plan to submit a PR tomorrow (control comments).
evgeniy-r: 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: I moved changes in checker to the separate PR: https://github.com/dotenv-linter/dotenv-linter/pull/281

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5dc8e25..10c8e7e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@ # Changelog
 
 ## [Unreleased]
 ### 🚀 Added
+- Add fixer: UnorderedKeyFixer [#261](https://github.com/dotenv-linter/dotenv-linter/pull/261) ([@evgeniy-r](https://github.com/evgeniy-r))
 - Add backup feature and `--no-backup` flag for fixers [#272](https://github.com/dotenv-linter/dotenv-linter/pull/272) ([@baile320](https://github.com/baile320))
 - Add fixer: ExtraBlankLineFixer [#260](https://github.com/dotenv-linter/dotenv-linter/pull/260) ([@diggymo](https://github.com/diggymo))
 - Add fixer: DuplicatedKeyFixer [#270](https://github.com/dotenv-linter/dotenv-linter/pull/270) ([@utter-step](https://github.com/utter-step))
diff --git a/src/fixes.rs b/src/fixes.rs
index 0a9c80d..e9c3326 100644
--- a/src/fixes.rs
+++ b/src/fixes.rs
@@ -10,6 +10,7 @@
 mod quote_character;
 mod space_character;
 mod trailing_whitespace;
+mod unordered_key;
 
 trait Fix {
     fn name(&self) -> &str;
@@ -49,11 +50,9 @@ fn fixlist() -> Vec<Box<dyn Fix>> {
         Box::new(quote_character::QuoteCharacterFixer::default()),
         Box::new(incorrect_delimiter::IncorrectDelimiterFixer::default()),
         Box::new(duplicated_key::DuplicatedKeyFixer::default()),
-        // Then we should run the fixers that handle the line entry collection at whole.
-        // And at the end we should run the fixer for ExtraBlankLine check (because the previous
-        // fixers can create additional extra blank lines).
         Box::new(extra_blank_line::ExtraBlankLineFixer::default()),
-        // UnorderedKeyFixer should be here
+        // Then we should run the fixers that handle the line entry collection at whole
+        Box::new(unordered_key::UnorderedKeyFixer::default()),
         Box::new(ending_blank_line::EndingBlankLineFixer::default()),
     ]
 }
diff --git a/src/fixes/unordered_key.rs b/src/fixes/unordered_key.rs
new file mode 100644
index 0000000..d6d9f60
--- /dev/null
+++ b/src/fixes/unordered_key.rs
@@ -0,0 +1,725 @@
+use super::Fix;
+use crate::common::*;
+
+pub(crate) struct UnorderedKeyFixer<'a> {
+    name: &'a str,
+}
+
+impl Default for UnorderedKeyFixer<'_> {
+    fn default() -> Self {
+        Self {
+            name: "UnorderedKey",
+        }
+    }
+}
+
+// When we sort the keys, we handle a significant line (with key) with all previous comments
+// as a whole. E. g.:
+// ```
+// B=C
+// # Comment
+// A=B
+// ```
+// will be fixed to:
+// ```
+// # Comment
+// A=B
+// B=C
+// ```
+// We check the order separately in each group of entries (groups are separated by blank lines or
+// control comments).
+impl Fix for UnorderedKeyFixer<'_> {
+    fn name(&self) -> &str {
+        self.name
+    }
+
+    fn fix_warnings(
+        &mut self,
+        warnings: Vec<&mut Warning>,
+        lines: &mut Vec<LineEntry>,
+    ) -> Option<usize> {
+        // We find all sorting groups and sort them
+
+        let mut start_index = 0;
+        let mut end = None;
+        let mut is_disabled = false;
+
+        for i in 0..lines.len() {
+            let line = &lines[i];
+
+            let mut is_control_comment = false;
+            let mut controls_this_check = false;
+            let mut is_off = false;
+
+            if let Some(comment) = line.get_control_comment() {
+                is_control_comment = true;
+                controls_this_check = comment.checks.contains(&self.name);
+                is_off = comment.is_disabled();
+            }
+
+            if !is_disabled {
+                if !line.is_empty_or_comment() {
+                    end.replace(i + 1);
+                }
+
+                if line.is_empty() || line.is_last_line() || is_control_comment {
+                    if let Some(end_index) = end {
+                        Self::sort_part(&mut lines[start_index..end_index]);
+                        end = None;
+                    }
+                    start_index = i + 1;
+                }
+            }
+
+            if controls_this_check {
+                is_disabled = is_off;
+                start_index = i + 1;
+            }
+        }
+
+        let count = warnings.len();
+        for warning in warnings {
+            warning.mark_as_fixed();
+        }
+
+        Some(count)
+    }
+}
+
+impl UnorderedKeyFixer<'_> {
+    fn sort_part(part: &mut [LineEntry]) {
+        // Each slice includes a significant line (with key) and previous comments (if present)
+        let mut slice_start = 0;
+        let mut slices = Vec::with_capacity(part.len());
+        for (i, line) in part.iter().enumerate() {
+            if !line.is_comment() {
+                slices.push(&part[slice_start..=i]);
+                slice_start = i + 1;
+            }
+        }
+
+        slices.sort_by_cached_key(|slice| {
+            // I think, that we should modify get_key() so it will return Option<&str> instead of
+            // Option<String>.
+            slice.last()?.get_key()
+        });
+
+        let mut sorted_lines = Vec::with_capacity(part.len());
+        for slice in slices {
+            sorted_lines.extend_from_slice(slice);
+        }
+
+        part.clone_from_slice(sorted_lines.as_slice());
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::common::tests::line_entry;
+
+    fn get_lines(lines: Vec<&str>) -> Vec<LineEntry> {
+        let total = lines.len();
+
+        lines
+            .iter()
+            .enumerate()
+            .map(|(i, &line)| line_entry(i + 1, total, line))
+            .collect()
+    }
+
+    fn get_warnings(lines: &[LineEntry], warnings: Vec<(usize, &str)>) -> Vec<Warning> {
+        warnings
+            .into_iter()
+            .map(|(i, line)| Warning::new(lines[i].clone(), "UnorderedKey", String::from(line)))
+            .collect()
+    }
+
+    fn run_fixer(warnings: &mut [Warning], lines: &mut Vec<LineEntry>) -> Option<usize> {
+        let mut fixer = UnorderedKeyFixer::default();
+        let warning_refs = warnings.iter_mut().collect();
+
+        fixer.fix_warnings(warning_refs, lines)
+    }
+
+    fn assert_lines(result: &[LineEntry], lines: Vec<&str>) {
+        for (i, &line) in lines.iter().enumerate() {
+            assert_eq!(line, result[i].raw_string.as_str());
+        }
+    }
+
+    #[test]
+    fn fix_warnings_test() {
+        let mut lines = get_lines(vec!["B=C", "A=B", "D=E", "\n"]);
+        let mut warnings = get_warnings(&lines, vec![(1, "The A key should go before B key")]);
+
+        assert_eq!(Some(1), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(&lines, vec!["A=B", "B=C", "D=E", "\n"]);
+
+        assert!(warnings[0].is_fixed);
+    }
+
+    #[test]
+    fn many_moves_test() {
+        let mut lines = get_lines(vec!["X=X", "A=A", "D=D", "Z=Z", "Y=Y", "KLM=123", "\n"]);
+        let mut warnings = get_warnings(
+            &lines,
+            vec![
+                (1, "The A key should go before B key"),
+                (4, "The Y key should go before Z key"),
+                (5, "The KLM key should go before Y key"),
+            ],
+        );
+
+        assert_eq!(Some(3), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(
+            &lines,
+            vec!["A=A", "D=D", "KLM=123", "X=X", "Y=Y", "Z=Z", "\n"],
+        );
+
+        for warning in warnings {
+            assert!(warning.is_fixed);
+        }
+    }
+
+    #[test]
+    fn previous_comments_test() {
+        let mut lines = get_lines(vec![
+            "# X comment 1",
+            "# X comment 2",
+            "X=X",
+            "# A comment",
+            "A=A",
+            "B=B",
+            "Z=Z",
+            "\n",
+        ]);
+        let mut warnings = get_warnings(&lines, vec![(4, "The A key should go before X key")]);
+
+        assert_eq!(Some(1), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(
+            &lines,
+            vec![
+                "# A comment",
+                "A=A",
+                "B=B",
+                "# X comment 1",
+                "# X comment 2",
+                "X=X",
+                "Z=Z",
+                "\n",
+            ],
+        );
+
+        assert!(warnings[0].is_fixed);
+    }
+
+    #[test]
+    fn several_groups_test() {
+        let mut lines = get_lines(vec![
+            "\n",
+            "# start comment",
+            "\n",
+            "A=1",
+            "E=3",
+            "C=2",
+            "\n",
+            "# middle comment",
+            "\n",
+            "D=2",
+            "# B comment",
+            "B=1",
+            "\n",
+            "AB=22",
+            "CD=33",
+            "\n",
+            "# end comment",
+            "\n",
+        ]);
+        let mut warnings = get_warnings(
+            &lines,
+            vec![
+                (6, "The C key should go before E key"),
+                (12, "The B key should go before D key"),
+            ],
+        );
+
+        assert_eq!(Some(2), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(
+            &lines,
+            vec![
+                "\n",
+                "# start comment",
+                "\n",
+                "A=1",
+                "C=2",
+                "E=3",
+                "\n",
+                "# middle comment",
+                "\n",
+                "# B comment",
+                "B=1",
+                "D=2",
+                "\n",
+                "AB=22",
+                "CD=33",
+                "\n",
+                "# end comment",
+                "\n",
+            ],
+        );
+
+        for warning in warnings {
+            assert!(warning.is_fixed);
+        }
+    }
+
+    #[test]
+    fn no_ending_blank_line_test() {
+        let mut lines = get_lines(vec!["B=C", "A=B", "D=E"]);
+        let mut warnings = get_warnings(&lines, vec![(2, "The A key should go before B key")]);
+
+        assert_eq!(Some(1), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(&lines, vec!["A=B", "B=C", "D=E"]);
+
+        assert!(warnings[0].is_fixed);
+    }
+
+    #[test]
+    fn all_file_control_comments_test() {
+        let mut lines = get_lines(vec![
+            "# dotenv-linter:off UnorderedKey",
+            "B=C",
+            "A=B",
+            "L=L",
+            "K=K",
+            "D=E",
+            "\n",
+        ]);
+        let mut warnings = vec![];
+
+        assert_eq!(Some(0), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(
+            &lines,
+            vec![
+                "# dotenv-linter:off UnorderedKey",
+                "B=C",
+                "A=B",
+                "L=L",
+                "K=K",
+                "D=E",
+                "\n",
+            ],
+        );
+    }
+
+    #[test]
+    fn some_lines_control_comments_test() {
+        let mut lines = get_lines(vec![
+            "C=C",
+            "B=B",
+            "# dotenv-linter:off UnorderedKey",
+            "A2=2",
+            "A1=1",
+            "# dotenv-linter:on UnorderedKey",
+            "X=X",
+            "A=A",
+            "\n",
+        ]);
+        let mut warnings = get_warnings(
+            &lines,
+            vec![
+                (2, "The B key should go before C key"),
+                (8, "The A key should go before X key"),
+            ],
+        );
+
+        assert_eq!(Some(2), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(
+            &lines,
+            vec![
+                "B=B",
+                "C=C",
+                "# dotenv-linter:off UnorderedKey",
+                "A2=2",
+                "A1=1",
+                "# dotenv-linter:on UnorderedKey",
+                "A=A",
+                "X=X",
+                "\n",
+            ],
+        );
+
+        for warning in warnings {
+            assert!(warning.is_fixed);
+        }
+    }
+
+    #[test]
+    fn other_checker_control_comments_test() {
+        let mut lines = get_lines(vec![
+            "C=C",
+            "B=B",
+            "# dotenv-linter:off LowercaseKey",
+            "A2=2",
+            "A1=1",
+            "# dotenv-linter:on LowercaseKey",
+            "X=X",
+            "A=A",
+            "\n",
+        ]);
+        let mut warnings = get_warnings(
+            &lines,
+            vec![
+                (2, "The B key should go before C key"),
+                (5, "The A1 key should go before A2 key"),
+                (8, "The A key should go before X key"),
+            ],
+        );
+
+        assert_eq!(Some(3), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(
+            &lines,
+            vec![
+                "B=B",
+                "C=C",
+                "# dotenv-linter:off LowercaseKey",
+                "A1=1",
+                "A2=2",
+                "# dotenv-linter:on LowercaseKey",
+                "A=A",
+                "X=X",
+                "\n",
+            ],
+        );
+
+        for warning in warnings {
+            assert!(warning.is_fixed);
+        }
+    }
+
+    #[test]
+    fn different_control_comments_test() {
+        let mut lines = get_lines(vec![
+            "C=C",
+            "B=B",
+            "# dotenv-linter:off UnorderedKey",
+            "A2=2",
+            "A1=1",
+            "# dotenv-linter:on UnorderedKey",
+            "# dotenv-linter:off LowercaseKey",
+            "B2=2",
+            "B1=1",
+            "# dotenv-linter:on LowercaseKey",
+            "# some comment",
+            "\n",
+        ]);
+        let mut warnings = get_warnings(
+            &lines,
+            vec![
+                (2, "The B key should go before C key"),
+                (9, "The B1 key should go before B2 key"),
+            ],
+        );
+
+        assert_eq!(Some(2), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(
+            &lines,
+            vec![
+                "B=B",
+                "C=C",
+                "# dotenv-linter:off UnorderedKey",
+                "A2=2",
+                "A1=1",
+                "# dotenv-linter:on UnorderedKey",
+                "# dotenv-linter:off LowercaseKey",
+                "B1=1",
+                "B2=2",
+                "# dotenv-linter:on LowercaseKey",
+                "# some comment",
+                "\n",
+            ],
+        );
+
+        for warning in warnings {
+            assert!(warning.is_fixed);
+        }
+    }
+
+    #[test]
+    fn comment_between_control_comments_test() {
+        let mut lines = get_lines(vec![
+            "# dotenv-linter:off UnorderedKey",
+            "A2=2",
+            "A1=1",
+            "# dotenv-linter:on UnorderedKey",
+            "# comment",
+            "# dotenv-linter:off LowercaseKey",
+            "B2=2",
+            "B1=1",
+            "# dotenv-linter:on LowercaseKey",
+            "\n",
+        ]);
+        let mut warnings = get_warnings(&lines, vec![(8, "The B1 key should go before B2 key")]);
+
+        assert_eq!(Some(1), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(
+            &lines,
+            vec![
+                "# dotenv-linter:off UnorderedKey",
+                "A2=2",
+                "A1=1",
+                "# dotenv-linter:on UnorderedKey",
+                "# comment",
+                "# dotenv-linter:off LowercaseKey",
+                "B1=1",
+                "B2=2",
+                "# dotenv-linter:on LowercaseKey",
+                "\n",
+            ],
+        );
+
+        assert!(warnings[0].is_fixed);
+    }
+
+    #[test]
+    fn crossed_control_comments_test() {
+        let mut lines = get_lines(vec![
+            "# dotenv-linter:off LowercaseKey",
+            "A3=3",
+            "A2=2",
+            "# dotenv-linter:off UnorderedKey",
+            "A1=1",
+            "# dotenv-linter:on LowercaseKey",
+            "B2=2",
+            "B1=1",
+            "# dotenv-linter:on UnorderedKey",
+            "C2=2",
+            "C1=1",
+            "\n",
+        ]);
+        let mut warnings = get_warnings(
+            &lines,
+            vec![
+                (3, "The A2 key should go before A3 key"),
+                (11, "The C1 key should go before C2 key"),
+            ],
+        );
+
+        assert_eq!(Some(2), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(
+            &lines,
+            vec![
+                "# dotenv-linter:off LowercaseKey",
+                "A2=2",
+                "A3=3",
+                "# dotenv-linter:off UnorderedKey",
+                "A1=1",
+                "# dotenv-linter:on LowercaseKey",
+                "B2=2",
+                "B1=1",
+                "# dotenv-linter:on UnorderedKey",
+                "C1=1",
+                "C2=2",
+                "\n",
+            ],
+        );
+
+        for warning in warnings {
+            assert!(warning.is_fixed);
+        }
+    }
+
+    #[test]
+    fn nested_control_comments_test() {
+        let mut lines = get_lines(vec![
+            "# dotenv-linter:off LowercaseKey",
+            "A4=4",
+            "# comment",
+            "A3=3",
+            "# dotenv-linter:off UnorderedKey",
+            "A2=2",
+            "A1=1",
+            "# dotenv-linter:on UnorderedKey",
+            "C2=2",
+            "C1=1",
+            "# dotenv-linter:on LowercaseKey",
+            "B2=2",
+            "B1=1",
+            "\n",
+        ]);
+        let mut warnings = get_warnings(
+            &lines,
+            vec![
+                (4, "The A3 key should go before A4 key"),
+                (10, "The C1 key should go before C2 key"),
+                (13, "The B1 key should go before B2 key"),
+            ],
+        );
+
+        assert_eq!(Some(3), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(
+            &lines,
+            vec![
+                "# dotenv-linter:off LowercaseKey",
+                "# comment",
+                "A3=3",
+                "A4=4",
+                "# dotenv-linter:off UnorderedKey",
+                "A2=2",
+                "A1=1",
+                "# dotenv-linter:on UnorderedKey",
+                "C1=1",
+                "C2=2",
+                "# dotenv-linter:on LowercaseKey",
+                "B1=1",
+                "B2=2",
+                "\n",
+            ],
+        );
+
+        for warning in warnings {
+            assert!(warning.is_fixed);
+        }
+    }
+
+    #[test]
+    fn empty_control_comments_test() {
+        let mut lines = get_lines(vec![
+            "Z=Z",
+            "# dotenv-linter:off UnorderedKey",
+            "# dotenv-linter:on UnorderedKey",
+            "A4=4",
+            "# comment",
+            "A2=2",
+            "A3=3",
+            "A1=1",
+            "\n",
+        ]);
+        let mut warnings = get_warnings(
+            &lines,
+            vec![
+                (6, "The A2 key should go before A4 key"),
+                (8, "The A1 key should go before A3 key"),
+            ],
+        );
+
+        assert_eq!(Some(2), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(
+            &lines,
+            vec![
+                "Z=Z",
+                "# dotenv-linter:off UnorderedKey",
+                "# dotenv-linter:on UnorderedKey",
+                "A1=1",
+                "# comment",
+                "A2=2",
+                "A3=3",
+                "A4=4",
+                "\n",
+            ],
+        );
+
+        for warning in warnings {
+            assert!(warning.is_fixed);
+        }
+    }
+
+    #[test]
+    fn control_comments_and_blank_lines_test() {
+        let mut lines = get_lines(vec![
+            "\n",
+            "# start comment",
+            "\n",
+            "A=1",
+            "E=3",
+            "# C comment",
+            "# C comment",
+            "C=2",
+            "\n",
+            "# middle comment",
+            "# dotenv-linter:off UnorderedKey",
+            "A201=201",
+            "A200=200",
+            "\n",
+            "A100=100",
+            "A101=101",
+            "# dotenv-linter:on UnorderedKey",
+            "W=2",
+            "D=2",
+            "# dotenv-linter:off KeyWithoutValue",
+            "# B comment",
+            "B=1",
+            "# dotenv-linter:on KeyWithoutValue",
+            "# dotenv-linter:off UnorderedKey",
+            "\n",
+            "AB=22",
+            "CD=33",
+            "\n",
+            "# end comment",
+            "\n",
+        ]);
+        let mut warnings = get_warnings(
+            &lines,
+            vec![
+                (8, "The C key should go before E key"),
+                (19, "The D key should go before W key"),
+            ],
+        );
+
+        assert_eq!(Some(2), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(
+            &lines,
+            vec![
+                "\n",
+                "# start comment",
+                "\n",
+                "A=1",
+                "# C comment",
+                "# C comment",
+                "C=2",
+                "E=3",
+                "\n",
+                "# middle comment",
+                "# dotenv-linter:off UnorderedKey",
+                "A201=201",
+                "A200=200",
+                "\n",
+                "A100=100",
+                "A101=101",
+                "# dotenv-linter:on UnorderedKey",
+                "D=2",
+                "W=2",
+                "# dotenv-linter:off KeyWithoutValue",
+                "# B comment",
+                "B=1",
+                "# dotenv-linter:on KeyWithoutValue",
+                "# dotenv-linter:off UnorderedKey",
+                "\n",
+                "AB=22",
+                "CD=33",
+                "\n",
+                "# end comment",
+                "\n",
+            ],
+        );
+
+        for warning in warnings {
+            assert!(warning.is_fixed);
+        }
+    }
+}


Test patch:
diff --git a/tests/common/mod.rs b/tests/common/mod.rs
index 2c348a1..2c344b9 100644
--- a/tests/common/mod.rs
+++ b/tests/common/mod.rs
@@ -127,18 +127,6 @@ pub fn test_command_fix_success(&self, expected_output: String) {
             .stdout(expected_output);
     }
 
-    /// Run the default CLI binary, with "-f", in this TestDir and check it fails.
-    pub fn test_command_fix_fail(&self, expected_output: String) {
-        let mut cmd = Self::init_cmd();
-        let canonical_current_dir = canonicalize(&self.current_dir).expect("canonical current dir");
-        cmd.current_dir(&canonical_current_dir)
-            .args(&["-f", "--no-backup"])
-            .assert()
-            .failure()
-            .code(1)
-            .stdout(expected_output);
-    }
-
     /// Run the default CLI binary, with command line arguments,
     /// in this TestDir and check it succeeds.
     ///
diff --git a/tests/fixes/mod.rs b/tests/fixes/mod.rs
index cde18c0..b398490 100644
--- a/tests/fixes/mod.rs
+++ b/tests/fixes/mod.rs
@@ -9,6 +9,7 @@
 mod leading_character;
 mod space_character;
 mod trailing_whitespace;
+mod unordered_key;
 
 #[test]
 fn correct_file() {
@@ -53,31 +54,6 @@ fn key_without_value() {
     testdir.close();
 }
 
-#[test]
-fn unfixed_warnings() {
-    let testdir = TestDir::new();
-    let testfile = testdir.create_testfile(".env", "A=DEF\nB=BAR \nX-Y=Z\nf=BAR\nA=FOO\n");
-
-    let expected_output = String::from(
-        "Fixed warnings:\n\
-        .env:2 TrailingWhitespace: Trailing whitespace detected\n\
-        .env:3 IncorrectDelimiter: The X-Y key has incorrect delimiter\n\
-        .env:4 LowercaseKey: The f key should be in uppercase\n\
-        .env:5 DuplicatedKey: The A key is duplicated\n\
-        \n\
-        Unfixed warnings:\n\
-       .env:5 UnorderedKey: The A key should go before the A key\n",
-    );
-    testdir.test_command_fix_fail(expected_output);
-
-    assert_eq!(
-        testfile.contents().as_str(),
-        "A=DEF\nB=BAR\nX_Y=Z\nF=BAR\n# A=FOO\n"
-    );
-
-    testdir.close();
-}
-
 #[test]
 fn multiple_files() {
     let testdir = TestDir::new();
@@ -89,17 +65,15 @@ fn multiple_files() {
     let expected_output = String::from(
         "Fixed warnings:\n\
         2.env:1 LowercaseKey: The abc key should be in uppercase\n\
+        2.env:4 UnorderedKey: The B key should go before the F key\n\
         3.env:1 TrailingWhitespace: Trailing whitespace detected\n\
         3.env:2 LowercaseKey: The ab key should be in uppercase\n\
-        3.env:4 DuplicatedKey: The A key is duplicated\n\
-        \n\
-        Unfixed warnings:\n\
-        2.env:4 UnorderedKey: The B key should go before the F key\n",
+        3.env:4 DuplicatedKey: The A key is duplicated\n",
     );
-    testdir.test_command_fix_fail(expected_output);
+    testdir.test_command_fix_success(expected_output);
 
     assert_eq!(testfile1.contents().as_str(), "AB=DEF\nD=BAR\n\nF=BAR\n");
-    assert_eq!(testfile2.contents().as_str(), "ABC=DEF\n\nF=BAR\nB=bbb\n");
+    assert_eq!(testfile2.contents().as_str(), "ABC=DEF\n\nB=bbb\nF=BAR\n");
     assert_eq!(testfile3.contents().as_str(), "A=b\nAB=DEF\n\n# A=c\n");
 
     testdir.close();
diff --git a/tests/fixes/unordered_key.rs b/tests/fixes/unordered_key.rs
new file mode 100644
index 0000000..5645ab7
--- /dev/null
+++ b/tests/fixes/unordered_key.rs
@@ -0,0 +1,64 @@
+use crate::common::TestDir;
+
+#[test]
+fn unordered_key() {
+    let testdir = TestDir::new();
+    let testfile = testdir.create_testfile(
+        ".env",
+        "# comment\n\
+        \n\
+        C=B\n\
+        # A comment\n\
+        A=B\n\
+        X=Y\n\
+        # more comments\n\
+        \n\
+        # middle comment\n\
+        \n\
+        M=123\n\
+        # K comment\n\
+        K=123\n\
+        # I comment\n\
+        # I comment\n\
+        I=123\n\
+        \n\
+        A1=1\n\
+        B1=1\n\
+        \n\
+        # end comment\n",
+    );
+    let expected_output = String::from(
+        "Fixed warnings:\n\
+        .env:5 UnorderedKey: The A key should go before the C key\n\
+        .env:13 UnorderedKey: The K key should go before the M key\n\
+        .env:16 UnorderedKey: The I key should go before the K key\n",
+    );
+    testdir.test_command_fix_success(expected_output);
+
+    assert_eq!(
+        testfile.contents().as_str(),
+        "# comment\n\
+        \n\
+        # A comment\n\
+        A=B\n\
+        C=B\n\
+        X=Y\n\
+        # more comments\n\
+        \n\
+        # middle comment\n\
+        \n\
+        # I comment\n\
+        # I comment\n\
+        I=123\n\
+        # K comment\n\
+        K=123\n\
+        M=123\n\
+        \n\
+        A1=1\n\
+        B1=1\n\
+        \n\
+        # end comment\n",
+    );
+
+    testdir.close();
+}