Repository: dotenv-linter/dotenv-linter
Pull request #259 (2020-08-14 01:31:29)

Implement LeadingCharacterFixer
This will close: https://github.com/dotenv-linter/dotenv-linter/issues/248

I have a couple comments/questions:
- When implementing this, I ran into issues with IncorrectDelimiter: it thought, for example, "*FOO=BAR" had an incorrect delimiter (this is ultimately fixed by LeadingCharacterFixer, as expected). IMO, that is not a delimiter at all, since delimiters should occur /between/ other characters, so I added a check for that in IncorrectDelimiterChecker (here: https://github.com/dotenv-linter/dotenv-linter/compare/master...baile320:leading-char-fixer?expand=1#diff-9d20ccc745eeab0a2cfbe87e10ee860cR27-R33), and removed an erroneous test.
- Depending on the leading characters, it is still possible to get erroneous UnorderedKeys warnings. I'm not sure the best way to handle this (I made no updates to that), and would like thoughts/suggestions on how to fix (if it should be fixed here) or if it's worthy of a separate PR. There's an illustrative example in my integration test (https://github.com/dotenv-linter/dotenv-linter/compare/master...baile320:leading-char-fixer?expand=1#diff-9aff8b00dfa64d0bf60b17a8b7fe7c20R4-R22)... as you can see, it throws no UnorderedKeys warnings, although it likely should, since after cleaning the keys are technically out of order (the issue is that the checking order includes the */./1 characters at the beginning of the keys, not the "cleaned" versions of the keys).

#### ✔ 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);
- [ ] Docs have been added / updated (for bug fixes / features).

mgrachev: > * When implementing this, I ran into issues with IncorrectDelimiter: it thought, for example, "*FOO=BAR" had an incorrect delimiter (this is ultimately fixed by LeadingCharacterFixer, as expected). IMO, that is not a delimiter at all, since delimiters should occur /between/ other characters, so I added a check for that in IncorrectDelimiterChecker (here: https://github.com/dotenv-linter/dotenv-linter/compare/master...baile320:leading-char-fixer?expand=1#diff-9d20ccc745eeab0a2cfbe87e10ee860cR27-R33), and removed an erroneous test.

Good catch! Thank you! 👍
But I'd like you to write a test for that case for the `IncorrectDelimiter`.

> * Depending on the leading characters, it is still possible to get erroneous UnorderedKeys warnings. I'm not sure the best way to handle this (I made no updates to that), and would like thoughts/suggestions on how to fix (if it should be fixed here) or if it's worthy of a separate PR. There's an illustrative example in my integration test (https://github.com/dotenv-linter/dotenv-linter/compare/master...baile320:leading-char-fixer?expand=1#diff-9aff8b00dfa64d0bf60b17a8b7fe7c20R4-R22)... as you can see, it throws no UnorderedKeys warnings, although it likely should, since after cleaning the keys are technically out of order (the issue is that the checking order includes the */./1 characters at the beginning of the keys, not the "cleaned" versions of the keys).

@evgeniy-r is already working to do it (https://github.com/dotenv-linter/dotenv-linter/issues/252)
DDtKey: > > * Depending on the leading characters, it is still possible to get erroneous UnorderedKeys warnings. I'm not sure the best way to handle this (I made no updates to that), and would like thoughts/suggestions on how to fix (if it should be fixed here) or if it's worthy of a separate PR. There's an illustrative example in my integration test (https://github.com/dotenv-linter/dotenv-linter/compare/master...baile320:leading-char-fixer?expand=1#diff-9aff8b00dfa64d0bf60b17a8b7fe7c20R4-R22)... as you can see, it throws no UnorderedKeys warnings, although it likely should, since after cleaning the keys are technically out of order (the issue is that the checking order includes the */./1 characters at the beginning of the keys, not the "cleaned" versions of the keys).
> 
> @evgeniy-r is already working on that feature (#252) muscle

And I think it is permissible to use the initially correct ordering in this integration test, since the target check does not order.

baile320: I merged in the changes from #267 and made some other relatively minor changes related to test names, checks, formatting. I think this PR is ready to go or be re-reviewed! Thanks!

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ce21406..4199fe0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@ # Changelog
 
 ## [Unreleased]
 ### 🚀 Added
+- Add fixer: LeadingCharacter [#259](https://github.com/dotenv-linter/dotenv-linter/pull/259) ([@baile320](https://github.com/baile320))
 - Add fixer: IncorrectDelimiter [#258]((https://github.com/dotenv-linter/dotenv-linter/pull/258) ([@gillespiecd](https://github.com/gillespiecd))
 - Add support of comments to disable checks [#239](https://github.com/dotenv-linter/dotenv-linter/pull/239) ([@mgrachev](https://github.com/mgrachev))
 - Add autofix for EndingBlankLine [#263](https://github.com/dotenv-linter/dotenv-linter/pull/263) ([@baile320](https://github.com/baile320))
diff --git a/src/checks/leading_character.rs b/src/checks/leading_character.rs
index 246fe96..a1b2730 100644
--- a/src/checks/leading_character.rs
+++ b/src/checks/leading_character.rs
@@ -47,7 +47,7 @@ mod tests {
     const MESSAGE: &str = "Invalid leading character detected";
 
     #[test]
-    fn normal() {
+    fn no_leading_chars_test() {
         let mut checker = LeadingCharacterChecker::default();
         let line = LineEntry {
             number: 1,
diff --git a/src/fixes.rs b/src/fixes.rs
index e05a79c..30501bb 100644
--- a/src/fixes.rs
+++ b/src/fixes.rs
@@ -3,6 +3,7 @@
 mod ending_blank_line;
 mod incorrect_delimiter;
 mod key_without_value;
+mod leading_character;
 mod lowercase_key;
 mod quote_character;
 mod space_character;
@@ -42,6 +43,7 @@ fn fixlist() -> Vec<Box<dyn Fix>> {
         Box::new(lowercase_key::LowercaseKeyFixer::default()),
         Box::new(space_character::SpaceCharacterFixer::default()),
         Box::new(trailing_whitespace::TrailingWhitespaceFixer::default()),
+        Box::new(leading_character::LeadingCharacterFixer::default()),
         Box::new(quote_character::QuoteCharacterFixer::default()),
         Box::new(incorrect_delimiter::IncorrectDelimiterFixer::default()),
         // Then we should run the fixers that handle the line entry collection at whole.
diff --git a/src/fixes/leading_character.rs b/src/fixes/leading_character.rs
new file mode 100644
index 0000000..15416e9
--- /dev/null
+++ b/src/fixes/leading_character.rs
@@ -0,0 +1,267 @@
+use super::Fix;
+use crate::common::*;
+
+pub(crate) struct LeadingCharacterFixer<'a> {
+    name: &'a str,
+}
+
+impl Default for LeadingCharacterFixer<'_> {
+    fn default() -> Self {
+        Self {
+            name: "LeadingCharacter",
+        }
+    }
+}
+
+impl Fix for LeadingCharacterFixer<'_> {
+    fn name(&self) -> &str {
+        self.name
+    }
+
+    fn fix_line(&self, line: &mut LineEntry) -> Option<()> {
+        let key = line.get_key()?;
+
+        let cleaned_key = remove_invalid_leading_chars(&key);
+
+        line.raw_string = format!("{}={}", cleaned_key, line.get_value()?);
+
+        Some(())
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::path::PathBuf;
+
+    #[test]
+    fn fix_leading_dot() {
+        let fixer = LeadingCharacterFixer::default();
+
+        let mut leading_period = LineEntry {
+            number: 2,
+            file: FileEntry {
+                path: PathBuf::from(".env"),
+                file_name: ".env".to_string(),
+                total_lines: 7,
+            },
+            raw_string: String::from(".FOO=BAR"),
+        };
+
+        assert_eq!(Some(()), fixer.fix_line(&mut leading_period));
+        assert_eq!("FOO=BAR", leading_period.raw_string);
+    }
+
+    #[test]
+    fn fix_leading_space() {
+        let fixer = LeadingCharacterFixer::default();
+
+        let mut leading_space = LineEntry {
+            number: 1,
+            file: FileEntry {
+                path: PathBuf::from(".env"),
+                file_name: ".env".to_string(),
+                total_lines: 7,
+            },
+            raw_string: String::from(" FOO=BAR"),
+        };
+
+        assert_eq!(Some(()), fixer.fix_line(&mut leading_space));
+        assert_eq!("FOO=BAR", leading_space.raw_string);
+    }
+
+    #[test]
+    fn fix_leading_asterisk() {
+        let fixer = LeadingCharacterFixer::default();
+
+        let mut leading_asterisk = LineEntry {
+            number: 3,
+            file: FileEntry {
+                path: PathBuf::from(".env"),
+                file_name: ".env".to_string(),
+                total_lines: 7,
+            },
+            raw_string: String::from("*FOO=BAR"),
+        };
+
+        assert_eq!(Some(()), fixer.fix_line(&mut leading_asterisk));
+        assert_eq!("FOO=BAR", leading_asterisk.raw_string);
+    }
+
+    #[test]
+    fn fix_leading_number() {
+        let fixer = LeadingCharacterFixer::default();
+
+        let mut leading_number = LineEntry {
+            number: 4,
+            file: FileEntry {
+                path: PathBuf::from(".env"),
+                file_name: ".env".to_string(),
+                total_lines: 7,
+            },
+            raw_string: String::from("1FOO=BAR"),
+        };
+
+        assert_eq!(Some(()), fixer.fix_line(&mut leading_number));
+        assert_eq!("FOO=BAR", leading_number.raw_string);
+    }
+
+    #[test]
+    fn fix_many_invalid_leading_chars() {
+        let fixer = LeadingCharacterFixer::default();
+
+        let mut leading_number = LineEntry {
+            number: 4,
+            file: FileEntry {
+                path: PathBuf::from(".env"),
+                file_name: ".env".to_string(),
+                total_lines: 7,
+            },
+            raw_string: String::from("-1&*FOO=BAR"),
+        };
+
+        assert_eq!(Some(()), fixer.fix_line(&mut leading_number));
+        assert_eq!("FOO=BAR", leading_number.raw_string);
+    }
+
+    #[test]
+    fn leading_underscore_is_unchanged() {
+        let fixer = LeadingCharacterFixer::default();
+        let mut leading_underscore = LineEntry {
+            number: 5,
+            file: FileEntry {
+                path: PathBuf::from(".env"),
+                file_name: ".env".to_string(),
+                total_lines: 7,
+            },
+            raw_string: String::from("_FOO=BAR"),
+        };
+
+        assert_eq!(Some(()), fixer.fix_line(&mut leading_underscore));
+        assert_eq!("_FOO=BAR", leading_underscore.raw_string);
+    }
+
+    #[test]
+    fn no_leading_char_is_unchanged() {
+        let fixer = LeadingCharacterFixer::default();
+
+        let mut normal = LineEntry {
+            number: 6,
+            file: FileEntry {
+                path: PathBuf::from(".env"),
+                file_name: ".env".to_string(),
+                total_lines: 7,
+            },
+            raw_string: String::from("FOO=BAR"),
+        };
+
+        assert_eq!(Some(()), fixer.fix_line(&mut normal));
+        assert_eq!("FOO=BAR", normal.raw_string);
+    }
+
+    #[test]
+    fn fix_warnings_test() {
+        let fixer = LeadingCharacterFixer::default();
+        let mut lines = vec![
+            LineEntry {
+                number: 1,
+                file: FileEntry {
+                    path: PathBuf::from(".env"),
+                    file_name: ".env".to_string(),
+                    total_lines: 7,
+                },
+                raw_string: String::from(".FOO=BAR"),
+            },
+            LineEntry {
+                number: 2,
+                file: FileEntry {
+                    path: PathBuf::from(".env"),
+                    file_name: ".env".to_string(),
+                    total_lines: 7,
+                },
+                raw_string: String::from(" Z=Y"),
+            },
+            LineEntry {
+                number: 3,
+                file: FileEntry {
+                    path: PathBuf::from(".env"),
+                    file_name: ".env".to_string(),
+                    total_lines: 7,
+                },
+                raw_string: String::from("*BAR=BAZ"),
+            },
+            LineEntry {
+                number: 4,
+                file: FileEntry {
+                    path: PathBuf::from(".env"),
+                    file_name: ".env".to_string(),
+                    total_lines: 7,
+                },
+                raw_string: String::from("1QUX=QUUX"),
+            },
+            LineEntry {
+                number: 5,
+                file: FileEntry {
+                    path: PathBuf::from(".env"),
+                    file_name: ".env".to_string(),
+                    total_lines: 7,
+                },
+                raw_string: String::from("_QUUX=FOOBAR"),
+            },
+            LineEntry {
+                number: 6,
+                file: FileEntry {
+                    path: PathBuf::from(".env"),
+                    file_name: ".env".to_string(),
+                    total_lines: 7,
+                },
+                raw_string: String::from("KEY=VALUE"),
+            },
+            LineEntry {
+                number: 7,
+                file: FileEntry {
+                    path: PathBuf::from(".env"),
+                    file_name: ".env".to_string(),
+                    total_lines: 7,
+                },
+                raw_string: String::from("\n"),
+            },
+        ];
+
+        let mut warnings = vec![
+            Warning::new(
+                lines[0].clone(),
+                "LeadingCharacter",
+                String::from("Invalid leading character detected"),
+            ),
+            Warning::new(
+                lines[1].clone(),
+                "LeadingCharacter",
+                String::from("Invalid leading character detected"),
+            ),
+            Warning::new(
+                lines[2].clone(),
+                "LeadingCharacter",
+                String::from("Invalid leading character detected"),
+            ),
+            Warning::new(
+                lines[3].clone(),
+                "LeadingCharacter",
+                String::from("Invalid leading character detected"),
+            ),
+        ];
+
+        assert_eq!(
+            Some(4),
+            fixer.fix_warnings(warnings.iter_mut().collect(), &mut lines)
+        );
+
+        assert_eq!("FOO=BAR", lines[0].raw_string);
+        assert_eq!("Z=Y", lines[1].raw_string);
+        assert_eq!("BAR=BAZ", lines[2].raw_string);
+        assert_eq!("QUX=QUUX", lines[3].raw_string);
+        assert_eq!("_QUUX=FOOBAR", lines[4].raw_string);
+        assert_eq!("KEY=VALUE", lines[5].raw_string);
+        assert_eq!("\n", lines[6].raw_string);
+    }
+}


Test patch:
diff --git a/tests/fixes/leading_character.rs b/tests/fixes/leading_character.rs
new file mode 100644
index 0000000..c8ab086
--- /dev/null
+++ b/tests/fixes/leading_character.rs
@@ -0,0 +1,22 @@
+use crate::common::TestDir;
+
+#[test]
+fn leading_character() {
+    let testdir = TestDir::new();
+    let testfile = testdir.create_testfile(".env", "*BAR=BAZ\n.ABC=DEF\n1QUX=QUUX\n_FOO=BAR\n");
+    let expected_output = String::from(
+        "Fixed warnings:\n\
+        .env:1 LeadingCharacter: Invalid leading character detected\n\
+        .env:2 LeadingCharacter: Invalid leading character detected\n\
+        .env:3 LeadingCharacter: Invalid leading character detected\n",
+    );
+
+    testdir.test_command_fix_success(expected_output);
+
+    assert_eq!(
+        testfile.contents().as_str(),
+        "BAR=BAZ\nABC=DEF\nQUX=QUUX\n_FOO=BAR\n"
+    );
+
+    testdir.close();
+}
diff --git a/tests/fixes/mod.rs b/tests/fixes/mod.rs
index 58bbd6c..0fc40e8 100644
--- a/tests/fixes/mod.rs
+++ b/tests/fixes/mod.rs
@@ -4,6 +4,7 @@
 
 mod ending_blank_line;
 mod incorrect_delimiter;
+mod leading_character;
 mod space_character;
 mod trailing_whitespace;