Repository: dotenv-linter/dotenv-linter
Pull request #282 (2020-09-18 12:26:30)

Add compare command
A proof of concept how comparing could work.
I think this isn't the cleanest implementation and needs some abstraction work in terms of types, and duplicate work between `compare` and `run` but I wanted to see if it can work and how it could work.

We didn't already agreed that this feature needed to be added to the tool, but I liked the idea, found some time and had fun playing around with it.

So this could be the starting point of a discussion and further investigation of that topic.

What do you think?

Check this feature:

```sh
# creating test data
echo HELLO=value >> .env
echo HELLO=value >> .env.local
echo WORLD=value >> .env

# running
dotenv-linter --compare .env .env.local
# or
cargo run -- --compare .env .env.local
```

mgrachev: @mstruebing Thank you for your contribution! 🔥 I like this feature too ❤️ 

I think we will merge this after release [v2.2.0](https://github.com/dotenv-linter/dotenv-linter/issues/238).

I have a few ideas how we can improve it, but I will come back with them a little later.


DDtKey: I think it would also make sense to do this as a separate subcommand after #333 🤔
What do you think? @dotenv-linter/core

This can also be done as a separate PR later on.  Just a suggestion)

 
mgrachev: > I think it would also make sense to do this as a separate subcommand after #333 🤔
> What do you think? @dotenv-linter/core
> 
> This can also be done as a separate PR later on. Just a suggestion)

To tell the truth, I would like to merge this PR after #333
mgrachev: > Multi-functional `dotenv-linter`👍

Yes, we are slowly moving towards this way 🙂
If you have ideas what else we can include in the next release, let me know 😉

mstruebing: I polished the PR a bit, feedback or desired changes are welcome
mstruebing: I think this PR is on hold until the subcommands are finished, right?
mgrachev: > I think this PR is on hold until the subcommands are finished, right?

Yes, you are right 🙂
mgrachev: > I think this PR is on hold until the subcommands are finished, right?

Subcommands are finished 🎉
mstruebing: I updated the PR right now as a subcommand
codecov-io: # [Codecov](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/282?src=pr&el=h1) Report
> Merging [#282](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/282?src=pr&el=desc) (89075e2) into [master](https://codecov.io/gh/dotenv-linter/dotenv-linter/commit/4db20d14fc7dfa7e85cf9693abb1a2c37aee092b?el=desc) (4db20d1) will **decrease** coverage by `1.61%`.
> The diff coverage is `14.58%`.

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

```diff
@@            Coverage Diff             @@
##           master     #282      +/-   ##
==========================================
- Coverage   96.97%   95.36%   -1.62%     
==========================================
  Files          34       34              
  Lines        2415     2459      +44     
==========================================
+ Hits         2342     2345       +3     
- Misses         73      114      +41     
```


| [Impacted Files](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/282?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/lib.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/282/diff?src=pr&el=tree#diff-c3JjL2xpYi5ycw==) | `73.75% <10.52%> (-23.44%)` | :arrow_down: |
| [src/main.rs](https://codecov.io/gh/dotenv-linter/dotenv-linter/pull/282/diff?src=pr&el=tree#diff-c3JjL21haW4ucnM=) | `73.58% <30.00%> (-10.14%)` | :arrow_down: |

------

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

mgrachev: @mstruebing Thank you!

Can you explain how this feature should work?
Also please give some examples.
mstruebing: Sure, so in general what it does:
You use the `compare` command and at least two files to compare (as I'm writing this I realize I need to make sure that you pass at least two files).
It then collects all keys of every environment file and compares if some of them are present in one file but not in the other.

I think this is handy for testing if different environments have some variables missing.

Also sometimes you have an `.env.sample` file in your repository. Which might get's updated and you miss to update your development environment file. Then you could quickly run `dotenv-linter compare .env.sample .env` and see which keys you might need to add. This is especially useful in bigger projects with lots of environment variables.

To try it out you can use the command in the first comment, just replace `--compare` with `compare`.

Do you have any further questions I did not answered?
mgrachev: > Do you have any further questions I did not answered?

Now I understand, thank you!
Before that I encountered an error with the exit after printing the first warning and it confused me 😅
mgrachev: @mstruebing Can you write some integration tests for this feature?
mstruebing: I fixed all your comments except output and did it in single commits to be easier to review, I will squash the commits once before we would merge it.

I will fix the output soon and also add integration tests.
mstruebing: I think the structs `CompareWarning` and `CompareFileType` can be unified somehow, but I need a break now :)
mgrachev: Right now the output of the `compare` command looks like this:
```bash
$ dotenv-linter compare .env .env.local .env2
Comparing .env
Comparing .env.local
Comparing .env2
".env" is missing keys: "OK, MULTILINE"
".env.local" is missing keys: "OK, MULTILINE, 1MULTILINE, FOO, ZOO, WORLD"
".env2" is missing keys: "1MULTILINE, HELLO, ZOO, WORLD"
```

I would like to see something like this:
```bash
$ dotenv-linter compare .env .env.local .env2
Comparing .env
".env" is missing keys: "MULTILINE, OK"

Comparing .env.local
".env.local" is missing keys: "FOO, 1MULTILINE, WORLD, MULTILINE, OK, ZOO"

Comparing .env2
".env2" is missing keys: "1MULTILINE, HELLO, WORLD, ZOO"
```

What do you think?
mstruebing: I would reduce this further to:

```sh
$ dotenv-linter compare .env .env.local .env2
.env is missing keys: MULTILINE, OK
.env.local is missing keys: FOO, 1MULTILINE, WORLD, MULTILINE, OK, ZOO
.env2 is missing keys: 1MULTILINE, HELLO, WORLD, ZOO
```
mstruebing: I think I adressed all of the review comments, still need to add some tests, however I have a problem now which I'm not able to fix:

```rs
        for line in lines {
            if let Some(key) = line.get_key() { // `line` does not live long enough: borrowed value does not live long enough
                all_keys.insert(key);
                keys.push(key)
            }
        }
```

What can I do to prevent this?
mgrachev: > What can I do to prevent this, I tried multiple things without success?

See my comments. I have sketched the fastest solution of your problem, but not the most efficient 😅
DDtKey: @dotenv-linter/core  we should remember to add this to the documentation ([dotenv-linter.github.io](https://github.com/dotenv-linter/dotenv-linter.github.io)) 
mstruebing: Sorry for the long delay, I just pushed:

* Tests for quiet mode
* simplifications by @DDtKey 

Is there anything else I need to do except documentation?
mgrachev: > Is there anything else I need to do except documentation?

I will add documentation for this command in https://github.com/dotenv-linter/dotenv-linter.github.io/issues/16.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 256de66..ad84ed9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@ # Changelog
 
 ## [Unreleased]
 ### 🚀 Added
+- Add `compare`-command [#282](https://github.com/dotenv-linter/dotenv-linter/pull/282) ([@mstruebing](https://github.com/mstruebing))
 - Add colored output feature and `--no-color` flag to disable colors [#307](https://github.com/dotenv-linter/dotenv-linter/pull/307) ([@Nikhil0487](https://github.com/Nikhil0487))
 - Display linted files when run [#311](https://github.com/dotenv-linter/dotenv-linter/pull/311) ([@Anthuang](https://github.com/anthuang))
 - Add export prefix support [#340](https://github.com/dotenv-linter/dotenv-linter/pull/340)([@skonik](https://github.com/skonik))
diff --git a/src/common.rs b/src/common.rs
index 15d311a..7f76b59 100644
--- a/src/common.rs
+++ b/src/common.rs
@@ -1,13 +1,17 @@
 pub(crate) mod comment;
+mod compare;
 mod file_entry;
 mod line_entry;
 pub(crate) mod output;
 mod warning;
 
 use colored::*;
+pub use compare::CompareFileType;
+pub use compare::CompareWarning;
 pub use file_entry::FileEntry;
 pub use line_entry::LineEntry;
 pub use output::check::CheckOutput;
+pub use output::compare::CompareOutput;
 pub use output::fix::FixOutput;
 pub use warning::Warning;
 
diff --git a/src/common/compare.rs b/src/common/compare.rs
new file mode 100644
index 0000000..d92f617
--- /dev/null
+++ b/src/common/compare.rs
@@ -0,0 +1,31 @@
+use std::fmt;
+use std::path::PathBuf;
+
+use crate::common::*;
+
+// A structure used to compare environment files
+pub struct CompareFileType {
+    pub path: PathBuf,
+    pub keys: Vec<String>,
+    pub missing: Vec<String>,
+}
+
+pub struct CompareWarning {
+    pub path: PathBuf,
+    pub missing_keys: Vec<String>,
+}
+
+impl fmt::Display for CompareWarning {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(
+            f,
+            "{}",
+            format!(
+                "{} is missing keys: {}",
+                self.path.display(),
+                self.missing_keys.join(", ")
+            )
+            .italic(),
+        )
+    }
+}
diff --git a/src/common/output/compare.rs b/src/common/output/compare.rs
new file mode 100644
index 0000000..137f197
--- /dev/null
+++ b/src/common/output/compare.rs
@@ -0,0 +1,24 @@
+use crate::common::{CompareWarning, FileEntry};
+
+pub struct CompareOutput {
+    // Quiet program output mode
+    is_quiet_mode: bool,
+}
+
+impl CompareOutput {
+    pub fn new(is_quiet_mode: bool) -> Self {
+        CompareOutput { is_quiet_mode }
+    }
+
+    /// Prints information about a file in process
+    pub fn print_processing_info(&self, file: &FileEntry) {
+        if !self.is_quiet_mode {
+            println!("Comparing {}", file);
+        }
+    }
+
+    /// Prints warnings without any additional information
+    pub fn print_warnings(&self, warnings: &[CompareWarning]) {
+        warnings.iter().for_each(|w| println!("{}", w))
+    }
+}
diff --git a/src/common/output/mod.rs b/src/common/output/mod.rs
index 4216636..8da4ff7 100644
--- a/src/common/output/mod.rs
+++ b/src/common/output/mod.rs
@@ -1,2 +1,3 @@
 pub mod check;
+pub mod compare;
 pub mod fix;
diff --git a/src/lib.rs b/src/lib.rs
index e870718..9a36bde 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1,5 +1,6 @@
 use crate::common::*;
 use std::collections::BTreeMap;
+use std::collections::HashSet;
 use std::error::Error;
 use std::path::PathBuf;
 
@@ -9,6 +10,7 @@
 mod fs_utils;
 
 pub use checks::available_check_names;
+use common::CompareWarning;
 
 pub fn check(args: &clap::ArgMatches, current_dir: &PathBuf) -> Result<usize, Box<dyn Error>> {
     let lines_map = get_lines(args, current_dir)?;
@@ -124,6 +126,68 @@ fn get_needed_file_paths(args: &clap::ArgMatches) -> Vec<PathBuf> {
     file_paths
 }
 
+// Compares if different environment files contains the same variables and
+// returns warnings if not
+pub fn compare(
+    args: &clap::ArgMatches,
+    current_dir: &PathBuf,
+) -> Result<Vec<CompareWarning>, Box<dyn Error>> {
+    let mut all_keys: HashSet<String> = HashSet::new();
+    let lines_map = get_lines(args, current_dir)?;
+    let output = CompareOutput::new(args.is_present("quiet"));
+
+    let mut warnings: Vec<CompareWarning> = Vec::new();
+    let mut files_to_compare: Vec<CompareFileType> = Vec::new();
+
+    // Nothing to check
+    if lines_map.is_empty() {
+        return Ok(warnings);
+    }
+
+    // Create CompareFileType structures for each file
+    for (_, (fe, strings)) in lines_map.into_iter().enumerate() {
+        output.print_processing_info(&fe);
+        let lines = get_line_entries(&fe, strings);
+        let mut keys: Vec<String> = Vec::new();
+
+        for line in lines {
+            if let Some(key) = line.get_key() {
+                all_keys.insert(key.to_string());
+                keys.push(key.to_string());
+            }
+        }
+
+        let file_to_compare: CompareFileType = CompareFileType {
+            path: fe.path,
+            keys,
+            missing: Vec::new(),
+        };
+
+        files_to_compare.push(file_to_compare);
+    }
+
+    // Create warnings if any file misses any key
+    for file in files_to_compare {
+        let missing_keys: Vec<_> = all_keys
+            .iter()
+            .filter(|key| !file.keys.contains(key))
+            .map(|key| key.to_owned())
+            .collect();
+
+        if !missing_keys.is_empty() {
+            let warning = CompareWarning {
+                path: file.path,
+                missing_keys,
+            };
+
+            warnings.push(warning)
+        }
+    }
+
+    output.print_warnings(&warnings);
+    Ok(warnings)
+}
+
 fn get_file_paths(
     dir_entries: Vec<PathBuf>,
     excludes: &[PathBuf],
diff --git a/src/main.rs b/src/main.rs
index 7440f77..578396e 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -3,6 +3,13 @@
 use std::ffi::OsStr;
 use std::{env, process};
 
+fn quiet_flag() -> clap::Arg<'static, 'static> {
+    Arg::with_name("quiet")
+        .short("q")
+        .long("quiet")
+        .help("Doesn't display additional information")
+}
+
 fn main() -> Result<(), Box<dyn Error>> {
     #[cfg(windows)]
     colored::control::set_virtual_terminal(true).ok();
@@ -33,6 +40,12 @@ fn main() -> Result<(), Box<dyn Error>> {
 
             process::exit(0);
         }
+        ("compare", Some(files)) => {
+            let warnings = dotenv_linter::compare(&files, &current_dir)?;
+            if warnings.is_empty() {
+                process::exit(0);
+            }
+        }
         _ => {
             eprintln!("unknown command");
         }
@@ -71,6 +84,21 @@ fn get_args(current_dir: &OsStr) -> clap::ArgMatches {
                 .usage("dotenv-linter fix [FLAGS] [OPTIONS] <input>...")
                 .about("Automatically fixes warnings"),
         )
+        .subcommand(
+            SubCommand::with_name("compare")
+                .setting(AppSettings::ColoredHelp)
+                .visible_alias("c")
+                .args(&vec![
+                    Arg::with_name("input")
+                        .help("Files to compare")
+                        .multiple(true)
+                        .min_values(2)
+                        .required(true),
+                    quiet_flag(),
+                ])
+                .about("Compares if files have the same keys")
+                .usage("dotenv-linter compare <files>..."),
+        )
         .get_matches()
 }
 
@@ -103,9 +131,6 @@ fn common_args(current_dir: &OsStr) -> Vec<Arg> {
         Arg::with_name("no-color")
             .long("no-color")
             .help("Turns off the colored output"),
-        Arg::with_name("quiet")
-            .short("q")
-            .long("quiet")
-            .help("Doesn't display additional information"),
+        quiet_flag(),
     ]
 }


Test patch:
diff --git a/tests/cli.rs b/tests/cli.rs
index cb0cf67..3c3c323 100644
--- a/tests/cli.rs
+++ b/tests/cli.rs
@@ -1,6 +1,7 @@
 mod args;
 mod checks;
 mod common;
+mod compare;
 mod fixes;
 mod flags;
 mod options;
diff --git a/tests/compare/compare.rs b/tests/compare/compare.rs
new file mode 100644
index 0000000..999fc9d
--- /dev/null
+++ b/tests/compare/compare.rs
@@ -0,0 +1,63 @@
+use crate::common::TestDir;
+
+#[test]
+fn files_with_same_environment_variables() {
+    let test_dir = TestDir::new();
+    let testfile_one = test_dir.create_testfile(".env1", "FOO=abc\nBAR=def");
+    let testfile_two = test_dir.create_testfile(".env2", "FOO=abc\nBAR=def");
+    let expected_output = format!("Comparing .env1\nComparing .env2\n");
+
+    test_dir.test_command_success_with_args(
+        &["compare", testfile_one.as_str(), testfile_two.as_str()],
+        expected_output,
+    );
+}
+
+#[test]
+fn files_with_same_environment_variables_in_quiet_mode() {
+    let test_dir = TestDir::new();
+    let testfile_one = test_dir.create_testfile(".env1", "FOO=abc\nBAR=def");
+    let testfile_two = test_dir.create_testfile(".env2", "FOO=abc\nBAR=def");
+    let expected_output = format!("");
+
+    test_dir.test_command_success_with_args(
+        &[
+            "compare",
+            "--quiet",
+            testfile_one.as_str(),
+            testfile_two.as_str(),
+        ],
+        expected_output,
+    );
+}
+
+#[test]
+fn files_with_different_environment_variables() {
+    let test_dir = TestDir::new();
+    let testfile_one = test_dir.create_testfile(".env1", "FOO=abc");
+    let testfile_two = test_dir.create_testfile(".env2", "FOO=abc\nBAR=def");
+    let expected_output = format!("Comparing .env1\nComparing .env2\n.env1 is missing keys: BAR\n");
+
+    test_dir.test_command_fail_with_args(
+        &["compare", testfile_one.as_str(), testfile_two.as_str()],
+        expected_output,
+    )
+}
+
+#[test]
+fn files_with_different_environment_variables_in_quiet_mode() {
+    let test_dir = TestDir::new();
+    let testfile_one = test_dir.create_testfile(".env1", "FOO=abc");
+    let testfile_two = test_dir.create_testfile(".env2", "FOO=abc\nBAR=def");
+    let expected_output = format!(".env1 is missing keys: BAR\n");
+
+    test_dir.test_command_fail_with_args(
+        &[
+            "compare",
+            "--quiet",
+            testfile_one.as_str(),
+            testfile_two.as_str(),
+        ],
+        expected_output,
+    )
+}
diff --git a/tests/compare/mod.rs b/tests/compare/mod.rs
new file mode 100644
index 0000000..e642045
--- /dev/null
+++ b/tests/compare/mod.rs
@@ -0,0 +1,1 @@
+mod compare;