Repository: dotenv-linter/dotenv-linter
Pull request #180 (2020-05-12 20:41:10)

ExtraBlankLine checker
@mgrachev, I still have to check `\n` when adding the last line entry (we discussed this earlier) because of the special case with extra blank line at the end of file.

Any ideas?

#### ✔ 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);
- [x] Docs have been added / updated (for bug fixes / features).

codecov-io: # [Codecov](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/180?src=pr&el=h1) Report
> Merging [#180](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/180?src=pr&el=desc) into [master](https://codecov.io/gh/dotenv-linter/dotenv-linter/commit/05325b3570bf16ddd5c34e34013e2a9ccaee72e9&el=desc) will **decrease** coverage by `0.13%`.
> The diff coverage is `96.00%`.

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

```diff
@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
- Coverage   97.50%   97.37%   -0.14%     
==========================================
  Files          13       14       +1     
  Lines        1043     1104      +61     
==========================================
+ Hits         1017     1075      +58     
- Misses         26       29       +3     
```


| [Impacted Files](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/180?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/checks/extra\_blank\_line.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/180/diff?src=pr&el=tree#diff-c3JjL2NoZWNrcy9leHRyYV9ibGFua19saW5lLnJz) | `93.18% <93.18%> (ø)` | |
| [src/checks.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/180/diff?src=pr&el=tree#diff-c3JjL2NoZWNrcy5ycw==) | `100.00% <100.00%> (ø)` | |
| [src/checks/key\_without\_value.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/180/diff?src=pr&el=tree#diff-c3JjL2NoZWNrcy9rZXlfd2l0aG91dF92YWx1ZS5ycw==) | `92.85% <100.00%> (+1.42%)` | :arrow_up: |
| [src/checks/leading\_character.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/180/diff?src=pr&el=tree#diff-c3JjL2NoZWNrcy9sZWFkaW5nX2NoYXJhY3Rlci5ycw==) | `96.47% <100.00%> (+0.36%)` | :arrow_up: |
| [src/lib.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/180/diff?src=pr&el=tree#diff-c3JjL2xpYi5ycw==) | `94.80% <100.00%> (+0.06%)` | :arrow_up: |

------

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

evgeniy-r: @mgrachev, take a look one more time, please.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 61d1fd4..3e007b9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@ # Changelog
 
 ## [Unreleased]
 ### 🚀 Added
+- Add check: ExtraBlankLine [#180](https://github.com/dotenv-linter/dotenv-linter/pull/180) ([@evgeniy-r](https://github.com/evgeniy-r))
 - Add check: EndingBlankLine [#170](https://github.com/dotenv-linter/dotenv-linter/pull/170) ([@evgeniy-r](https://github.com/evgeniy-r))
 - Add check: Quote characters [#174](https://github.com/dotenv-linter/dotenv-linter/pull/174) ([@sourabhmarathe](https://github.com/sourabhmarathe))
 - Github Actions: Add caching in the CI workflow [#163](https://github.com/dotenv-linter/dotenv-linter/pull/163) ([@evgeniy-r](https://github.com/evgeniy-r))
diff --git a/README.md b/README.md
index b125098..c8fb303 100644
--- a/README.md
+++ b/README.md
@@ -29,6 +29,7 @@
 <p>
 &nbsp;&nbsp;&nbsp;&nbsp;✅<a href="#duplicated-key">Duplicated Key</a><br />
 &nbsp;&nbsp;&nbsp;&nbsp;✅<a href="#ending-blank-line-will-be-available-in-v200">Ending Blank Line</a> (will be available in <a href="https://github.com/dotenv-linter/dotenv-linter/issues/172">v2.0.0</a>)<br />
+&nbsp;&nbsp;&nbsp;&nbsp;✅<a href="#extra-blank-line-will-be-available-in-v200">Extra Blank Line</a> (will be available in <a href="https://github.com/dotenv-linter/dotenv-linter/issues/172">v2.0.0</a>)<br />
 &nbsp;&nbsp;&nbsp;&nbsp;✅<a href="#incorrect-delimiter">Incorrect delimiter</a><br />
 &nbsp;&nbsp;&nbsp;&nbsp;✅<a href="#key-without-value">Key without value</a><br />
 &nbsp;&nbsp;&nbsp;&nbsp;✅<a href="#leading-character">Leading character</a><br />
@@ -200,6 +201,41 @@ ### Ending Blank Line (will be available in [v2.0.0](https://github.com/dotenv-l
 
 ```
 
+### Extra Blank Line (will be available in [v2.0.0](https://github.com/dotenv-linter/dotenv-linter/issues/172))
+
+Detects if a file contains more than one blank line in a row.
+
+```env
+❌Wrong
+A=B
+
+
+FOO=BAR
+```
+
+```env
+❌Wrong
+A=B
+FOO=BAR
+
+
+```
+
+```env
+✅Correct
+A=B
+
+FOO=BAR
+
+```
+
+```env
+✅Correct
+A=B
+FOO=BAR
+
+```
+
 ### Incorrect delimiter
 
 Detects if a key does not use an underscore to separate words:
diff --git a/src/checks.rs b/src/checks.rs
index f247c59..52ac7e4 100644
--- a/src/checks.rs
+++ b/src/checks.rs
@@ -2,6 +2,7 @@
 
 mod duplicated_key;
 mod ending_blank_line;
+mod extra_blank_line;
 mod incorrect_delimiter;
 mod key_without_value;
 mod leading_character;
@@ -19,6 +20,7 @@ trait Check {
 fn checklist() -> Vec<Box<dyn Check>> {
     vec![
         Box::new(duplicated_key::DuplicatedKeyChecker::default()),
+        Box::new(extra_blank_line::ExtraBlankLineChecker::default()),
         Box::new(incorrect_delimiter::IncorrectDelimiterChecker::default()),
         Box::new(leading_character::LeadingCharacterChecker::default()),
         Box::new(key_without_value::KeyWithoutValueChecker::default()),
@@ -33,19 +35,19 @@ pub fn run(lines: Vec<LineEntry>) -> Vec<Warning> {
     let mut checks = checklist();
     let mut warnings: Vec<Warning> = Vec::new();
 
-    if let Some((last_line, rest_lines)) = lines.split_last() {
-        for line in rest_lines {
-            if line.is_empty_or_comment() {
-                continue;
-            }
+    for line in &lines {
+        if line.is_comment() {
+            continue;
+        }
 
-            for ch in &mut checks {
-                if let Some(warning) = ch.run(line) {
-                    warnings.push(warning);
-                }
+        for ch in &mut checks {
+            if let Some(warning) = ch.run(line) {
+                warnings.push(warning);
             }
         }
+    }
 
+    if let Some(last_line) = lines.last() {
         let mut ending_line_checker = ending_blank_line::EndingBlankLineChecker::default();
         if let Some(warning) = ending_line_checker.run(last_line) {
             warnings.push(warning);
@@ -86,7 +88,7 @@ fn run_with_empty_vec_test() {
 
     #[test]
     fn run_with_empty_line_test() {
-        let lines: Vec<LineEntry> = vec![line_entry(1, ""), blank_entry(2)];
+        let lines: Vec<LineEntry> = vec![blank_entry(1)];
         let expected: Vec<Warning> = Vec::new();
 
         assert_eq!(expected, run(lines));
@@ -94,7 +96,7 @@ fn run_with_empty_line_test() {
 
     #[test]
     fn run_with_comment_line_test() {
-        let lines: Vec<LineEntry> = vec![line_entry(1, "# Comment"), blank_entry(2)];
+        let lines: Vec<LineEntry> = vec![line_entry(1, "# Comment = 'Value'"), blank_entry(2)];
         let expected: Vec<Warning> = Vec::new();
 
         assert_eq!(expected, run(lines));
diff --git a/src/checks/extra_blank_line.rs b/src/checks/extra_blank_line.rs
new file mode 100644
index 0000000..7c098d3
--- /dev/null
+++ b/src/checks/extra_blank_line.rs
@@ -0,0 +1,89 @@
+use crate::checks::Check;
+use crate::common::*;
+
+pub(crate) struct ExtraBlankLineChecker {
+    template: &'static str,
+    name: &'static str,
+    last_blank_number: Option<usize>,
+}
+
+impl ExtraBlankLineChecker {
+    fn message(&self) -> String {
+        return format!("{}: {}", self.name, self.template);
+    }
+}
+
+impl Default for ExtraBlankLineChecker {
+    fn default() -> Self {
+        Self {
+            name: "ExtraBlankLine",
+            template: "Extra blank line detected",
+            last_blank_number: None,
+        }
+    }
+}
+
+impl Check for ExtraBlankLineChecker {
+    fn run(&mut self, line: &LineEntry) -> Option<Warning> {
+        if !line.is_empty() {
+            return None;
+        }
+
+        if let Some(last_blank_number) = self.last_blank_number {
+            if last_blank_number + 1 == line.number {
+                return Some(Warning::new(line.clone(), self.message()));
+            }
+        }
+        self.last_blank_number = Some(line.number);
+
+        None
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::path::PathBuf;
+
+    fn run_asserts(asserts: Vec<(&str, Option<&str>)>) {
+        let mut checker = ExtraBlankLineChecker::default();
+
+        for (i, assert) in asserts.iter().enumerate() {
+            let (content, message) = *assert;
+            let line = LineEntry {
+                number: i + 1,
+                file_path: PathBuf::from(".env"),
+                raw_string: String::from(content),
+            };
+            let expected = message.map(|msg| Warning::new(line.clone(), String::from(msg)));
+
+            assert_eq!(checker.run(&line), expected);
+        }
+    }
+
+    #[test]
+    fn no_blank_lines() {
+        let asserts = vec![("A=B", None), ("C=D", None)];
+
+        run_asserts(asserts);
+    }
+
+    #[test]
+    fn single_blank_line() {
+        let asserts = vec![("A=B", None), ("", None), ("C=D", None)];
+
+        run_asserts(asserts);
+    }
+
+    #[test]
+    fn two_blank_lines() {
+        let asserts = vec![
+            ("A=B", None),
+            ("", None),
+            ("", Some("ExtraBlankLine: Extra blank line detected")),
+            ("C=D", None),
+        ];
+
+        run_asserts(asserts);
+    }
+}
diff --git a/src/checks/key_without_value.rs b/src/checks/key_without_value.rs
index 99ff39b..92d9db4 100644
--- a/src/checks/key_without_value.rs
+++ b/src/checks/key_without_value.rs
@@ -23,7 +23,7 @@ fn message(&self, key: &str) -> String {
 
 impl Check for KeyWithoutValueChecker<'_> {
     fn run(&mut self, line: &LineEntry) -> Option<Warning> {
-        if !line.raw_string.contains('=') {
+        if !(line.is_empty() || line.raw_string.contains('=')) {
             Some(Warning::new(line.clone(), self.message(&line.raw_string)))
         } else {
             None
@@ -47,6 +47,17 @@ fn working_run_with_value() {
         assert_eq!(None, checker.run(&line));
     }
 
+    #[test]
+    fn working_run_with_blank_line() {
+        let mut checker = KeyWithoutValueChecker::default();
+        let line = LineEntry {
+            number: 1,
+            file_path: PathBuf::from(".env"),
+            raw_string: String::from(""),
+        };
+        assert_eq!(None, checker.run(&line));
+    }
+
     #[test]
     fn working_run_without_value() {
         let mut checker = KeyWithoutValueChecker::default();
diff --git a/src/checks/leading_character.rs b/src/checks/leading_character.rs
index 745d92a..c5424b4 100644
--- a/src/checks/leading_character.rs
+++ b/src/checks/leading_character.rs
@@ -23,9 +23,10 @@ fn message(&self) -> String {
 
 impl Check for LeadingCharacterChecker {
     fn run(&mut self, line: &LineEntry) -> Option<Warning> {
-        if line
-            .raw_string
-            .starts_with(|c: char| c.is_alphabetic() || c == '_')
+        if line.is_empty()
+            || line
+                .raw_string
+                .starts_with(|c: char| c.is_alphabetic() || c == '_')
         {
             None
         } else {
@@ -52,6 +53,17 @@ fn normal() {
         assert_eq!(None, checker.run(&line));
     }
 
+    #[test]
+    fn blank_line() {
+        let mut checker = LeadingCharacterChecker::default();
+        let line = LineEntry {
+            number: 1,
+            file_path: PathBuf::from(".env"),
+            raw_string: String::from(""),
+        };
+        assert_eq!(None, checker.run(&line));
+    }
+
     #[test]
     fn leading_underscore() {
         let mut checker = LeadingCharacterChecker::default();
diff --git a/src/lib.rs b/src/lib.rs
index 78df02f..305e754 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -62,13 +62,16 @@ fn run_checks(fe: &FileEntry) -> io::Result<Vec<Warning>> {
     if f.seek(SeekFrom::End(-ending_seq_length)).is_ok()
         && f.read_to_string(&mut last_line)? == ending_seq_length as usize
     {
-        // We add a special LineEntry with the final character in the file
-        // for the "Ending Blank Line" check.
-        lines.push(LineEntry {
-            number: number + 1,
-            file_path: fe.path.clone(),
-            raw_string: last_line,
-        })
+        // We add an one more LineEntry with the final character in the file
+        // for the "Ending Blank Line" check if this final character is the "\n"
+        // (the latter condition is needed because of "Extra Blank Line" check at the end of file).
+        if last_line.as_str() == "\n" {
+            lines.push(LineEntry {
+                number: number + 1,
+                file_path: fe.path.clone(),
+                raw_string: last_line,
+            });
+        }
     }
 
     Ok(checks::run(lines))


Test patch:
diff --git a/tests/ending_blank_line.rs b/tests/ending_blank_line.rs
index 6fbc5c9..67af352 100644
--- a/tests/ending_blank_line.rs
+++ b/tests/ending_blank_line.rs
@@ -28,7 +28,7 @@ fn incorrect_files() {
         "A=B",
         "# Comment 1\n# Comment 2\n# Comment 3",
     ];
-    let expected_line_numbers = vec![4, 4, 2, 4];
+    let expected_line_numbers = vec![3, 3, 1, 3];
 
     for (i, content) in contents.iter().enumerate() {
         let testdir = TestDir::new();
diff --git a/tests/extra_blank_line.rs b/tests/extra_blank_line.rs
new file mode 100644
index 0000000..c8b02c4
--- /dev/null
+++ b/tests/extra_blank_line.rs
@@ -0,0 +1,69 @@
+#[allow(dead_code)]
+mod cli_common;
+
+use cli_common::TestDir;
+
+#[test]
+fn correct_files() {
+    let contents = vec![
+        "A=B\nF=BAR\n\nFOO=BAR\n",
+        "A=B\r\nF=BAR\r\n\r\nFOO=BAR\r\n",
+        "\n# comment\n\nABC=DEF\n",
+    ];
+
+    for content in contents {
+        let testdir = TestDir::new();
+        let testfile = testdir.create_testfile(".env", content);
+        let args = &[testfile.as_str()];
+
+        testdir.test_command_success_with_args(args);
+    }
+}
+
+#[test]
+fn two_blank_lines_at_the_beginning() {
+    let content = "\n\nABC=DEF\nD=BAR\nFOO=BAR\n";
+
+    let testdir = TestDir::new();
+    let testfile = testdir.create_testfile(".env", content);
+    let args = &[testfile.as_str()];
+    let expected_output = format!(
+        "{}:{} ExtraBlankLine: Extra blank line detected\n",
+        testfile.shortname_as_str(),
+        2
+    );
+
+    testdir.test_command_fail_with_args(args, expected_output);
+}
+
+#[test]
+fn two_blank_lines_in_the_middle() {
+    let content = "ABC=DEF\nD=BAR\n\n\nFOO=BAR\n";
+
+    let testdir = TestDir::new();
+    let testfile = testdir.create_testfile(".env", content);
+    let args = &[testfile.as_str()];
+    let expected_output = format!(
+        "{}:{} ExtraBlankLine: Extra blank line detected\n",
+        testfile.shortname_as_str(),
+        4
+    );
+
+    testdir.test_command_fail_with_args(args, expected_output);
+}
+
+#[test]
+fn two_blank_lines_at_the_end() {
+    let content = "ABC=DEF\nD=BAR\nFOO=BAR\n\n";
+
+    let testdir = TestDir::new();
+    let testfile = testdir.create_testfile(".env", content);
+    let args = &[testfile.as_str()];
+    let expected_output = format!(
+        "{}:{} ExtraBlankLine: Extra blank line detected\n",
+        testfile.shortname_as_str(),
+        5
+    );
+
+    testdir.test_command_fail_with_args(args, expected_output);
+}