Repository: dotenv-linter/dotenv-linter
Pull request #287 (2020-09-26 08:43:42)

Finishing autofix
This PR fixes the issue #285.
There is a little duplication, but I think that is OK for now.

As far as I can see, the autofix feature is finally completed.

#### ✔ Checklist:

- [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).


codecov-commenter: # [Codecov](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/287?src=pr&el=h1) Report
> Merging [#287](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/287?src=pr&el=desc) into [master](https://codecov.io/gh/dotenv-linter/dotenv-linter/commit/36b904eebed695157ac0d810edbd1e1036687124?el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `96.29%`.

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

```diff
@@            Coverage Diff             @@
##           master     #287      +/-   ##
==========================================
- Coverage   96.76%   96.74%   -0.03%     
==========================================
  Files          32       32              
  Lines        2195     2240      +45     
==========================================
+ Hits         2124     2167      +43     
- Misses         71       73       +2     
```


| [Impacted Files](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/287?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/fixes/duplicated\_key.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/287/diff?src=pr&el=tree#diff-c3JjL2ZpeGVzL2R1cGxpY2F0ZWRfa2V5LnJz) | `96.36% <90.90%> (-3.64%)` | :arrow_down: |
| [src/fixes.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/287/diff?src=pr&el=tree#diff-c3JjL2ZpeGVzLnJz) | `95.45% <100.00%> (+0.95%)` | :arrow_up: |
| [src/lib.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/287/diff?src=pr&el=tree#diff-c3JjL2xpYi5ycw==) | `95.58% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/287?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/287?src=pr&el=footer). Last update [36b904e...8b1147e](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/287?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

evgeniy-r: @mgrachev, I remember you mentioned that we should remove the `Fixed/Unfixed` feature.

I can do this, it will simplify the code.
mgrachev: > @mgrachev, I remember you mentioned that we should remove the `Fixed/Unfixed` feature.
> 
> 
> 
> I can do this, it will simplify the code.

I have already done it, but haven't published yet.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 10c8e7e..ed4710f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -24,6 +24,7 @@ ### 🚀 Added
 - Add installation CI test for Windows (via `install.sh`) [#235](https://github.com/dotenv-linter/dotenv-linter/pull/235) ([@DDtKey](https://github.com/DDtKey))
 
 ### 🔧 Changed
+- Fix a bug with fixers spawning new warnings [#287](https://github.com/dotenv-linter/dotenv-linter/pull/287) ([@evgeniy-r](https://github.com/evgeniy-r))
 - Fix a bug with `UnorderedKeyChecker` and control comments [#283](https://github.com/dotenv-linter/dotenv-linter/pull/283) ([@mgrachev](https://github.com/mgrachev))
 - Change the line grouping for the `UnorderedKey` checker [#281](https://github.com/dotenv-linter/dotenv-linter/pull/281) ([@evgeniy-r](https://github.com/evgeniy-r))
 - Fix a bug with `ExtraBlankLineFixer` and control comments [#279](https://github.com/dotenv-linter/dotenv-linter/pull/279) ([@mgrachev](https://github.com/mgrachev))
diff --git a/src/fixes.rs b/src/fixes.rs
index e9c3326..43c65d1 100644
--- a/src/fixes.rs
+++ b/src/fixes.rs
@@ -49,28 +49,33 @@ fn fixlist() -> Vec<Box<dyn Fix>> {
         Box::new(leading_character::LeadingCharacterFixer::default()),
         Box::new(quote_character::QuoteCharacterFixer::default()),
         Box::new(incorrect_delimiter::IncorrectDelimiterFixer::default()),
-        Box::new(duplicated_key::DuplicatedKeyFixer::default()),
         Box::new(extra_blank_line::ExtraBlankLineFixer::default()),
         // Then we should run the fixers that handle the line entry collection at whole
         Box::new(unordered_key::UnorderedKeyFixer::default()),
+        Box::new(duplicated_key::DuplicatedKeyFixer::default()),
         Box::new(ending_blank_line::EndingBlankLineFixer::default()),
     ]
 }
 
-pub fn run(warnings: &mut [Warning], lines: &mut Vec<LineEntry>) -> usize {
+pub fn run(warnings: &mut [Warning], lines: &mut Vec<LineEntry>, skip_checks: &[&str]) -> usize {
     if warnings.is_empty() {
         return 0;
     }
+    let mut fixes = fixlist();
+
+    // Skip fixes for checks in --skip argument (globally)
+    fixes.retain(|f| !skip_checks.contains(&f.name()));
 
     let mut count = 0;
-    for mut fixer in fixlist() {
+    for mut fixer in fixes {
         // We can optimize it: create check_name:warnings map in advance
         let fixer_warnings: Vec<&mut Warning> = warnings
             .iter_mut()
             .filter(|w| w.check_name == fixer.name())
             .collect();
 
-        if !fixer_warnings.is_empty() {
+        // Some fixers are mandatory because previous fixers can spawn warnings for them
+        if ["DuplicatedKey", "UnorderedKey"].contains(&fixer.name()) || !fixer_warnings.is_empty() {
             match fixer.fix_warnings(fixer_warnings, lines) {
                 Some(fixer_count) => count += fixer_count,
                 None => {
@@ -99,7 +104,7 @@ fn run_with_empty_warnings_test() {
         let mut lines = vec![line_entry(1, 2, "A=B"), blank_line_entry(2, 2)];
         let mut warnings: Vec<Warning> = Vec::new();
 
-        assert_eq!(0, run(&mut warnings, &mut lines));
+        assert_eq!(0, run(&mut warnings, &mut lines, &[]));
     }
 
     #[test]
@@ -115,7 +120,7 @@ fn run_with_fixable_warning_test() {
             String::from("The c key should be in uppercase"),
         )];
 
-        assert_eq!(1, run(&mut warnings, &mut lines));
+        assert_eq!(1, run(&mut warnings, &mut lines, &[]));
         assert_eq!("C=d", lines[1].raw_string);
         assert!(warnings[0].is_fixed);
     }
@@ -133,7 +138,7 @@ fn run_with_unfixable_warning_test() {
             String::from("The UNFIXABLE- key is not fixable"),
         )];
 
-        assert_eq!(0, run(&mut warnings, &mut lines));
+        assert_eq!(0, run(&mut warnings, &mut lines, &[]));
         assert!(!warnings[0].is_fixed);
     }
 
@@ -157,10 +162,100 @@ fn run_when_lines_do_not_fit_numbers_test() {
             ),
         ];
 
-        assert_eq!(0, run(&mut warnings, &mut lines));
+        assert_eq!(0, run(&mut warnings, &mut lines, &[]));
         assert!(!warnings[0].is_fixed);
     }
 
+    #[test]
+    fn new_warnings_after_fix_test() {
+        let mut lines = vec![
+            line_entry(1, 5, "A1=1"),
+            line_entry(2, 5, "A2=2"),
+            line_entry(3, 5, "a0=0"),
+            line_entry(4, 5, "a2=2"),
+            blank_line_entry(5, 5),
+        ];
+        let mut warnings = vec![
+            Warning::new(
+                lines[2].clone(),
+                "LowercaseKey",
+                String::from("The a0 key should be in uppercase"),
+            ),
+            Warning::new(
+                lines[3].clone(),
+                "LowercaseKey",
+                String::from("The a2 key should be in uppercase"),
+            ),
+        ];
+
+        assert_eq!(2, run(&mut warnings, &mut lines, &[]));
+        assert_eq!("A0=0", lines[0].raw_string);
+        assert_eq!("A1=1", lines[1].raw_string);
+        assert_eq!("A2=2", lines[2].raw_string);
+        assert_eq!("# A2=2", lines[3].raw_string);
+        assert_eq!("\n", lines[4].raw_string);
+    }
+
+    #[test]
+    fn skip_duplicated_key() {
+        let mut lines = vec![
+            line_entry(1, 5, "A1=1"),
+            line_entry(2, 5, "A2=2"),
+            line_entry(3, 5, "a0=0"),
+            line_entry(4, 5, "a2=2"),
+            blank_line_entry(5, 5),
+        ];
+        let mut warnings = vec![
+            Warning::new(
+                lines[2].clone(),
+                "LowercaseKey",
+                String::from("The a0 key should be in uppercase"),
+            ),
+            Warning::new(
+                lines[3].clone(),
+                "LowercaseKey",
+                String::from("The a2 key should be in uppercase"),
+            ),
+        ];
+
+        assert_eq!(2, run(&mut warnings, &mut lines, &["DuplicatedKey"]));
+        assert_eq!("A0=0", lines[0].raw_string);
+        assert_eq!("A1=1", lines[1].raw_string);
+        assert_eq!("A2=2", lines[2].raw_string);
+        assert_eq!("A2=2", lines[3].raw_string);
+        assert_eq!("\n", lines[4].raw_string);
+    }
+
+    #[test]
+    fn skip_unordered_key() {
+        let mut lines = vec![
+            line_entry(1, 5, "A1=1"),
+            line_entry(2, 5, "A2=2"),
+            line_entry(3, 5, "a0=0"),
+            line_entry(4, 5, "a2=2"),
+            blank_line_entry(5, 5),
+        ];
+        let mut warnings = vec![
+            Warning::new(
+                lines[2].clone(),
+                "LowercaseKey",
+                String::from("The a0 key should be in uppercase"),
+            ),
+            Warning::new(
+                lines[3].clone(),
+                "LowercaseKey",
+                String::from("The a2 key should be in uppercase"),
+            ),
+        ];
+
+        assert_eq!(2, run(&mut warnings, &mut lines, &["UnorderedKey"]));
+        assert_eq!("A1=1", lines[0].raw_string);
+        assert_eq!("A2=2", lines[1].raw_string);
+        assert_eq!("A0=0", lines[2].raw_string);
+        assert_eq!("# A2=2", lines[3].raw_string);
+        assert_eq!("\n", lines[4].raw_string);
+    }
+
     struct TestFixer<'a> {
         name: &'a str,
     }
diff --git a/src/fixes/duplicated_key.rs b/src/fixes/duplicated_key.rs
index 428877e..f2a1ec4 100644
--- a/src/fixes/duplicated_key.rs
+++ b/src/fixes/duplicated_key.rs
@@ -1,5 +1,6 @@
 use super::Fix;
 use crate::common::*;
+use std::collections::HashSet;
 
 pub(crate) struct DuplicatedKeyFixer<'a> {
     name: &'a str,
@@ -18,6 +19,41 @@ fn name(&self) -> &str {
         self.name
     }
 
+    fn fix_warnings(
+        &mut self,
+        warnings: Vec<&mut Warning>,
+        lines: &mut Vec<LineEntry>,
+    ) -> Option<usize> {
+        let mut keys = HashSet::with_capacity(lines.len());
+        let mut is_disabled = false;
+
+        for line in lines {
+            if let Some(comment) = line.get_control_comment() {
+                if comment.checks.contains(&self.name) {
+                    is_disabled = comment.is_disabled();
+                }
+            }
+            if is_disabled {
+                continue;
+            }
+
+            if let Some(key) = line.get_key() {
+                if keys.contains(&key) {
+                    self.fix_line(line)?;
+                } else {
+                    keys.insert(key);
+                }
+            }
+        }
+
+        let count = warnings.len();
+        for warning in warnings {
+            warning.mark_as_fixed();
+        }
+
+        Some(count)
+    }
+
     fn fix_line(&mut self, line: &mut LineEntry) -> Option<()> {
         line.raw_string = format!("# {}", line.raw_string);
 
@@ -67,4 +103,96 @@ fn fix_warnings() {
 
         assert!(warnings.iter().all(|w| w.is_fixed));
     }
+
+    #[test]
+    fn fix_lines_without_warnings() {
+        let mut fixer = DuplicatedKeyFixer::default();
+        let mut lines = vec![
+            line_entry(1, 4, "FOO=BAR"),
+            line_entry(2, 4, "FOO=BAZ"),
+            line_entry(3, 4, "Z=Y"),
+            line_entry(4, 4, "Z=X"),
+        ];
+        let mut warnings = vec![];
+
+        assert_eq!(
+            Some(0),
+            fixer.fix_warnings(warnings.iter_mut().collect(), &mut lines)
+        );
+        assert_eq!("FOO=BAR", lines[0].raw_string);
+        assert_eq!("# FOO=BAZ", lines[1].raw_string);
+        assert_eq!("Z=Y", lines[2].raw_string);
+        assert_eq!("# Z=X", lines[3].raw_string);
+    }
+
+    #[test]
+    fn control_comment_at_first_line() {
+        let mut fixer = DuplicatedKeyFixer::default();
+        let mut lines = vec![
+            line_entry(1, 5, "# dotenv-linter:off DuplicatedKey"),
+            line_entry(2, 5, "FOO=BAR"),
+            line_entry(3, 5, "FOO=BAZ"),
+            line_entry(4, 5, "Z=Y"),
+            line_entry(5, 5, "Z=X"),
+        ];
+        let mut warnings = vec![];
+
+        assert_eq!(
+            Some(0),
+            fixer.fix_warnings(warnings.iter_mut().collect(), &mut lines)
+        );
+        assert_eq!("# dotenv-linter:off DuplicatedKey", lines[0].raw_string);
+        assert_eq!("FOO=BAR", lines[1].raw_string);
+        assert_eq!("FOO=BAZ", lines[2].raw_string);
+        assert_eq!("Z=Y", lines[3].raw_string);
+        assert_eq!("Z=X", lines[4].raw_string);
+    }
+
+    #[test]
+    fn control_comment_in_the_middle() {
+        let mut fixer = DuplicatedKeyFixer::default();
+        let mut lines = vec![
+            line_entry(1, 6, "FOO=BAR"),
+            line_entry(2, 6, "# dotenv-linter:off DuplicatedKey"),
+            line_entry(3, 6, "FOO=BAZ"),
+            line_entry(4, 6, "Z=Y"),
+            line_entry(5, 6, "# dotenv-linter:on DuplicatedKey"),
+            line_entry(6, 6, "Z=X"),
+        ];
+        let mut warnings = vec![];
+
+        assert_eq!(
+            Some(0),
+            fixer.fix_warnings(warnings.iter_mut().collect(), &mut lines)
+        );
+        assert_eq!("FOO=BAR", lines[0].raw_string);
+        assert_eq!("# dotenv-linter:off DuplicatedKey", lines[1].raw_string);
+        assert_eq!("FOO=BAZ", lines[2].raw_string);
+        assert_eq!("Z=Y", lines[3].raw_string);
+        assert_eq!("# dotenv-linter:on DuplicatedKey", lines[4].raw_string);
+        assert_eq!("Z=X", lines[5].raw_string);
+    }
+
+    #[test]
+    fn unrelated_control_comment() {
+        let mut fixer = DuplicatedKeyFixer::default();
+        let mut lines = vec![
+            line_entry(1, 5, "# dotenv-linter:off LowercaseKey"),
+            line_entry(2, 5, "FOO=BAR"),
+            line_entry(3, 5, "FOO=BAZ"),
+            line_entry(4, 5, "Z=Y"),
+            line_entry(5, 5, "Z=X"),
+        ];
+        let mut warnings = vec![];
+
+        assert_eq!(
+            Some(0),
+            fixer.fix_warnings(warnings.iter_mut().collect(), &mut lines)
+        );
+        assert_eq!("# dotenv-linter:off LowercaseKey", lines[0].raw_string);
+        assert_eq!("FOO=BAR", lines[1].raw_string);
+        assert_eq!("# FOO=BAZ", lines[2].raw_string);
+        assert_eq!("Z=Y", lines[3].raw_string);
+        assert_eq!("# Z=X", lines[4].raw_string);
+    }
 }
diff --git a/src/fixes/unordered_key.rs b/src/fixes/unordered_key.rs
index d6d9f60..c00ea68 100644
--- a/src/fixes/unordered_key.rs
+++ b/src/fixes/unordered_key.rs
@@ -160,6 +160,16 @@ fn fix_warnings_test() {
         assert!(warnings[0].is_fixed);
     }
 
+    #[test]
+    fn fix_when_no_warnings_test() {
+        let mut lines = get_lines(vec!["B=C", "A=B", "D=E", "\n"]);
+        let mut warnings = get_warnings(&lines, vec![]);
+
+        assert_eq!(Some(0), run_fixer(&mut warnings, &mut lines));
+
+        assert_lines(&lines, vec!["A=B", "B=C", "D=E", "\n"]);
+    }
+
     #[test]
     fn many_moves_test() {
         let mut lines = get_lines(vec!["X=X", "A=A", "D=D", "Z=Z", "Y=Y", "KLM=123", "\n"]);
diff --git a/src/lib.rs b/src/lib.rs
index 0bafd1f..1323c2d 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -54,7 +54,7 @@ pub fn run(args: &clap::ArgMatches, current_dir: &PathBuf) -> Result<Vec<Warning
 
         // run fixers & write results to file
         let mut result = checks::run(&lines, &skip_checks);
-        if is_fix && fixes::run(&mut result, &mut lines) > 0 {
+        if is_fix && !result.is_empty() && fixes::run(&mut result, &mut lines, &skip_checks) > 0 {
             // create backup copy unless user specifies not to
             let should_backup = !args.is_present("no-backup");
             if should_backup {


Test patch:
diff --git a/tests/common/mod.rs b/tests/common/mod.rs
index 2c344b9..8155009 100644
--- a/tests/common/mod.rs
+++ b/tests/common/mod.rs
@@ -127,6 +127,23 @@ pub fn test_command_fix_success(&self, expected_output: String) {
             .stdout(expected_output);
     }
 
+    /// Run the default CLI binary, with "-f" and other provided arguments,
+    /// in this TestDir and check it succeeds.
+    pub fn test_command_fix_success_with_args<I, S>(&self, expected_output: String, ext_args: I)
+    where
+        I: IntoIterator<Item = S>,
+        S: AsRef<OsStr>,
+    {
+        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"])
+            .args(ext_args)
+            .assert()
+            .success()
+            .stdout(expected_output);
+    }
+
     /// Run the default CLI binary, with command line arguments,
     /// in this TestDir and check it succeeds.
     ///
diff --git a/tests/fixes/key_without_value.rs b/tests/fixes/key_without_value.rs
new file mode 100644
index 0000000..edcc64f
--- /dev/null
+++ b/tests/fixes/key_without_value.rs
@@ -0,0 +1,16 @@
+use crate::common::TestDir;
+
+#[test]
+fn key_without_value() {
+    let testdir = TestDir::new();
+    let testfile = testdir.create_testfile(".env", "FOO\n\nBAR=\n\nBAZ=QUX\n");
+    let expected_output = String::from(
+        "Fixed warnings:\n\
+        .env:1 KeyWithoutValue: The FOO key should be with a value or have an equal sign\n",
+    );
+    testdir.test_command_fix_success(expected_output);
+
+    assert_eq!(testfile.contents().as_str(), "FOO=\n\nBAR=\n\nBAZ=QUX\n");
+
+    testdir.close();
+}
diff --git a/tests/fixes/leading_character.rs b/tests/fixes/leading_character.rs
index c8ab086..ba0481a 100644
--- a/tests/fixes/leading_character.rs
+++ b/tests/fixes/leading_character.rs
@@ -15,7 +15,7 @@ fn leading_character() {
 
     assert_eq!(
         testfile.contents().as_str(),
-        "BAR=BAZ\nABC=DEF\nQUX=QUUX\n_FOO=BAR\n"
+        "ABC=DEF\nBAR=BAZ\nQUX=QUUX\n_FOO=BAR\n"
     );
 
     testdir.close();
diff --git a/tests/fixes/lowercase_key.rs b/tests/fixes/lowercase_key.rs
new file mode 100644
index 0000000..12645a0
--- /dev/null
+++ b/tests/fixes/lowercase_key.rs
@@ -0,0 +1,17 @@
+use crate::common::TestDir;
+
+#[test]
+fn lowercase_key() {
+    let testdir = TestDir::new();
+    let testfile = testdir.create_testfile(".env", "abc=DEF\n\nfOO=BAR\n");
+    let expected_output = String::from(
+        "Fixed warnings:\n\
+        .env:1 LowercaseKey: The abc key should be in uppercase\n\
+        .env:3 LowercaseKey: The fOO key should be in uppercase\n",
+    );
+    testdir.test_command_fix_success(expected_output);
+
+    assert_eq!(testfile.contents().as_str(), "ABC=DEF\n\nFOO=BAR\n");
+
+    testdir.close();
+}
diff --git a/tests/fixes/mod.rs b/tests/fixes/mod.rs
index b398490..3c75afe 100644
--- a/tests/fixes/mod.rs
+++ b/tests/fixes/mod.rs
@@ -6,7 +6,9 @@
 mod ending_blank_line;
 mod extra_blank_line;
 mod incorrect_delimiter;
+mod key_without_value;
 mod leading_character;
+mod lowercase_key;
 mod space_character;
 mod trailing_whitespace;
 mod unordered_key;
@@ -24,32 +26,22 @@ fn correct_file() {
 }
 
 #[test]
-fn lowercase_key() {
+fn skip_checks() {
     let testdir = TestDir::new();
-    let testfile = testdir.create_testfile(".env", "abc=DEF\n\nfOO=BAR\n");
+    let testfile = testdir.create_testfile(".env", "A1=1\nA2=2\na0=0\na2=2\n");
+
     let expected_output = String::from(
         "Fixed warnings:\n\
-        .env:1 LowercaseKey: The abc key should be in uppercase\n\
-        .env:3 LowercaseKey: The fOO key should be in uppercase\n",
+        .env:3 LowercaseKey: The a0 key should be in uppercase\n\
+        .env:4 LowercaseKey: The a2 key should be in uppercase\n",
     );
-    testdir.test_command_fix_success(expected_output);
-
-    assert_eq!(testfile.contents().as_str(), "ABC=DEF\n\nFOO=BAR\n");
-
-    testdir.close();
-}
 
-#[test]
-fn key_without_value() {
-    let testdir = TestDir::new();
-    let testfile = testdir.create_testfile(".env", "FOO\n\nBAR=\n\nBAZ=QUX\n");
-    let expected_output = String::from(
-        "Fixed warnings:\n\
-        .env:1 KeyWithoutValue: The FOO key should be with a value or have an equal sign\n",
+    testdir.test_command_fix_success_with_args(
+        expected_output,
+        &["--skip", "DuplicatedKey", "UnorderedKey"],
     );
-    testdir.test_command_fix_success(expected_output);
 
-    assert_eq!(testfile.contents().as_str(), "FOO=\n\nBAR=\n\nBAZ=QUX\n");
+    assert_eq!(testfile.contents().as_str(), "A1=1\nA2=2\nA0=0\nA2=2\n");
 
     testdir.close();
 }