Repository: dotenv-linter/dotenv-linter
Pull request #260 (2020-08-14 13:17:59)

add ExtraBlankLineFixer (#260)
<!--
Thank you for your pull request. Please provide a description above and review
the requirements below.

Bug fixes and new features should include tests.
-->

#245 

Below are the points I would like you to check in particular.
- Unnecessary function (`test_command_fix_fail`) came out, was it better to delete it? 

<!-- _Please make sure to review and check all of these items:_ -->

#### ✔ 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);

<!-- _NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open._ -->


codecov-commenter: # [Codecov](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/260?src=pr&el=h1) Report
> Merging [#260](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/260?src=pr&el=desc) into [master](https://codecov.io/gh/dotenv-linter/dotenv-linter/commit/d326dcdec0414975c8cee0cced8aaf027740e107&el=desc) will **increase** coverage by `0.08%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master     #260      +/-   ##
==========================================
+ Coverage   97.21%   97.30%   +0.08%     
==========================================
  Files          21       23       +2     
  Lines        1869     2041     +172     
==========================================
+ Hits         1817     1986     +169     
- Misses         52       55       +3     
```


| [Impacted Files](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/260?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/fixes.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/260/diff?src=pr&el=tree#diff-c3JjL2ZpeGVzLnJz) | `96.84% <ø> (-3.16%)` | :arrow_down: |
| [src/fixes/extract\_blank\_line.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/260/diff?src=pr&el=tree#diff-c3JjL2ZpeGVzL2V4dHJhY3RfYmxhbmtfbGluZS5ycw==) | `100.00% <100.00%> (ø)` | |
| [src/fixes/quote\_character.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/260/diff?src=pr&el=tree#diff-c3JjL2ZpeGVzL3F1b3RlX2NoYXJhY3Rlci5ycw==) | `100.00% <0.00%> (ø)` | |
| [src/checks/quote\_character.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/260/diff?src=pr&el=tree#diff-c3JjL2NoZWNrcy9xdW90ZV9jaGFyYWN0ZXIucnM=) | `96.77% <0.00%> (+0.52%)` | :arrow_up: |

------

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

diggymo: I'm sorry for immatuate. I fixed thone now.
mgrachev: @diggymo Please, update your PR from the master branch.
diggymo: > @diggymo Please, update your PR from the master branch.

Sorry for being late. Just fixed.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1cd7e92..5b28600 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@ # Changelog
 
 ## [Unreleased]
 ### 🚀 Added
+- 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))
 - 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))
diff --git a/src/fixes.rs b/src/fixes.rs
index a34cbba..9cc3872 100644
--- a/src/fixes.rs
+++ b/src/fixes.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;
@@ -50,6 +51,7 @@ fn fixlist() -> Vec<Box<dyn Fix>> {
         // 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()),
         Box::new(duplicated_key::DuplicatedKeyFixer::default()),
         Box::new(ending_blank_line::EndingBlankLineFixer::default()),
     ]
diff --git a/src/fixes/extra_blank_line.rs b/src/fixes/extra_blank_line.rs
new file mode 100644
index 0000000..42a2a5d
--- /dev/null
+++ b/src/fixes/extra_blank_line.rs
@@ -0,0 +1,176 @@
+use super::Fix;
+use crate::common::*;
+
+pub(crate) struct ExtraBlankLineFixer<'a> {
+    name: &'a str,
+}
+
+impl Default for ExtraBlankLineFixer<'_> {
+    fn default() -> Self {
+        Self {
+            name: "ExtraBlankLine",
+        }
+    }
+}
+
+impl Fix for ExtraBlankLineFixer<'_> {
+    fn name(&self) -> &str {
+        self.name
+    }
+
+    fn fix_warnings(
+        &self,
+        warnings: Vec<&mut Warning>,
+        lines: &mut Vec<LineEntry>,
+    ) -> Option<usize> {
+        let warning_count = warnings.len();
+
+        // check and remove all blank lines.
+        lines.dedup_by(|a, b| a.is_empty() && b.is_empty());
+
+        // mark as fixed
+        for warning in warnings {
+            warning.mark_as_fixed();
+        }
+
+        Some(warning_count)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::path::PathBuf;
+
+    #[test]
+    fn no_blank_lines_test() {
+        let fixer = ExtraBlankLineFixer::default();
+        let file = FileEntry {
+            path: PathBuf::from(".env"),
+            file_name: ".env".to_string(),
+            total_lines: 3,
+        };
+
+        let warnings = vec![];
+        let lines = vec![
+            LineEntry {
+                number: 1,
+                file: file.clone(),
+                raw_string: String::from("FOO=BAR"),
+            },
+            LineEntry {
+                number: 2,
+                file: file.clone(),
+                raw_string: String::from(""),
+            },
+            LineEntry {
+                number: 3,
+                file: file.clone(),
+                raw_string: String::from("HOGE=HUGA"),
+            },
+        ];
+        let mut fixing_lines = lines.clone();
+
+        assert_eq!(Some(0), fixer.fix_warnings(warnings, &mut fixing_lines));
+        assert_eq!(lines, fixing_lines);
+    }
+
+    #[test]
+    fn fix_one_extra_blank_line_test() {
+        let fixer = ExtraBlankLineFixer::default();
+        let file = FileEntry {
+            path: PathBuf::from(".env"),
+            file_name: ".env".to_string(),
+            total_lines: 4,
+        };
+
+        let line1 = LineEntry {
+            number: 1,
+            file: file.clone(),
+            raw_string: String::from("FOO=BAR"),
+        };
+        let line2 = LineEntry {
+            number: 2,
+            file: file.clone(),
+            raw_string: String::from(""),
+        };
+        let line3 = LineEntry {
+            number: 3,
+            file: file.clone(),
+            raw_string: String::from(""),
+        };
+        let line4 = LineEntry {
+            number: 4,
+            file: file.clone(),
+            raw_string: String::from("HOGE=HUGA"),
+        };
+        let mut warning = Warning::new(
+            line3.clone(),
+            "ExtraBlankLine",
+            "Extra blank line detected".to_string(),
+        );
+        let warnings = vec![&mut warning];
+        let mut lines = vec![line1.clone(), line2.clone(), line3.clone(), line4.clone()];
+        assert_eq!(Some(1), fixer.fix_warnings(warnings, &mut lines));
+        assert!(warning.is_fixed);
+        assert_eq!(vec!(line1.clone(), line2.clone(), line4.clone()), lines);
+    }
+
+    #[test]
+    fn fix_multiple_blank_lines_test() {
+        let fixer = ExtraBlankLineFixer::default();
+        let file = FileEntry {
+            path: PathBuf::from(".env"),
+            file_name: ".env".to_string(),
+            total_lines: 4,
+        };
+
+        let line1 = LineEntry {
+            number: 1,
+            file: file.clone(),
+            raw_string: String::from("FOO=BAR"),
+        };
+        let line2 = LineEntry {
+            number: 2,
+            file: file.clone(),
+            raw_string: String::from(""),
+        };
+        let line3 = LineEntry {
+            number: 3,
+            file: file.clone(),
+            raw_string: String::from(""),
+        };
+        let line4 = LineEntry {
+            number: 4,
+            file: file.clone(),
+            raw_string: String::from(""),
+        };
+        let line5 = LineEntry {
+            number: 5,
+            file: file.clone(),
+            raw_string: String::from("HOGE=HUGA"),
+        };
+        let mut warning1 = Warning::new(
+            line3.clone(),
+            "ExtraBlankLine",
+            "Extra blank line detected".to_string(),
+        );
+        let mut warning2 = Warning::new(
+            line4.clone(),
+            "ExtraBlankLine",
+            "Extra blank line detected".to_string(),
+        );
+        let warnings = vec![&mut warning1, &mut warning2];
+        let mut lines = vec![
+            line1.clone(),
+            line2.clone(),
+            line3.clone(),
+            line4.clone(),
+            line5.clone(),
+        ];
+        assert_eq!(Some(2), fixer.fix_warnings(warnings, &mut lines));
+        assert!(warning1.is_fixed);
+        assert!(warning2.is_fixed);
+        assert_eq!(vec!(line1.clone(), line2.clone(), line5.clone()), lines);
+    }
+}


Test patch:
diff --git a/tests/fixes/extra_blank_line.rs b/tests/fixes/extra_blank_line.rs
new file mode 100644
index 0000000..dcf9078
--- /dev/null
+++ b/tests/fixes/extra_blank_line.rs
@@ -0,0 +1,16 @@
+use crate::common::TestDir;
+
+#[test]
+fn extra_blank_line() {
+    let testdir = TestDir::new();
+    let testfile = testdir.create_testfile(".env", "ABC=DEF\n\n\nFOO=BAR\n");
+    let expected_output = String::from(
+        "Fixed warnings:\n\
+        .env:3 ExtraBlankLine: Extra blank line detected\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 b415f32..cde18c0 100644
--- a/tests/fixes/mod.rs
+++ b/tests/fixes/mod.rs
@@ -4,6 +4,7 @@
 
 mod duplicated_key;
 mod ending_blank_line;
+mod extra_blank_line;
 mod incorrect_delimiter;
 mod leading_character;
 mod space_character;
@@ -55,22 +56,23 @@ fn key_without_value() {
 #[test]
 fn unfixed_warnings() {
     let testdir = TestDir::new();
-    let testfile = testdir.create_testfile(".env", "A=DEF\nB=BAR \nX-Y=Z\nf=BAR\n\n");
+    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:6 ExtraBlankLine: Extra blank line detected\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\n"
+        "A=DEF\nB=BAR\nX_Y=Z\nF=BAR\n# A=FOO\n"
     );
 
     testdir.close();
@@ -81,23 +83,24 @@ fn multiple_files() {
     let testdir = TestDir::new();
 
     let testfile1 = testdir.create_testfile("1.env", "AB=DEF\nD=BAR\n\nF=BAR\n");
-    let testfile2 = testdir.create_testfile("2.env", "abc=DEF\n\nF=BAR\n");
-    let testfile3 = testdir.create_testfile("3.env", "A=b \nab=DEF\n\n");
+    let testfile2 = testdir.create_testfile("2.env", "abc=DEF\n\nF=BAR\nB=bbb\n");
+    let testfile3 = testdir.create_testfile("3.env", "A=b \nab=DEF\n\nA=c\n");
 
     let expected_output = String::from(
         "Fixed warnings:\n\
         2.env:1 LowercaseKey: The abc key should be in uppercase\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\
-        3.env:4 ExtraBlankLine: Extra blank line detected\n",
+        2.env:4 UnorderedKey: The B key should go before the F key\n",
     );
     testdir.test_command_fix_fail(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\n");
-    assert_eq!(testfile3.contents().as_str(), "A=b\nAB=DEF\n\n");
+    assert_eq!(testfile2.contents().as_str(), "ABC=DEF\n\nF=BAR\nB=bbb\n");
+    assert_eq!(testfile3.contents().as_str(), "A=b\nAB=DEF\n\n# A=c\n");
 
     testdir.close();
 }