Repository: dotenv-linter/dotenv-linter
Pull request #223 (2020-06-26 15:54:22)

Recursive search for `.env` files
I think it is useful that the linter can search for all `env` files in the specified directory, including the subdirectories.
This does not prevent specifying specific files and directories input, as well as excluding them.

What do you think?

<!--
Thank you for your pull request. Please provide a description above and review
the requirements below.

Bug fixes and new features should include tests.
-->

<!-- _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)
- [X] Docs have been added / updated (for bug fixes / features).

<!-- _NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open._ -->


DDtKey: I found #87 issue, it's already discussed, but in fact, I'm not sure what to compare the linter with [libraries for load environment variables](https://github.com/dotenv-linter/dotenv-linter/issues/87#issuecomment-590050476) is correctly.

Most of the linters that I know by default still know how to work recursively.
We can add a flag to turn off recursion or turn it on, as an option, but it seems to me that this approach is more suitable for a linter.

I will be glad to discuss this and look at it from another perspective.
mstruebing: Like in #87 my mind didn't change.
I think for linters to be most usable they should check recursively but if there are valid reasons against I don't know.
One thing we wouldn't can do as other linters mostly do is to only check version control system tracked files, because `.env`-files usually are not version tracked. 
mgrachev: @DDtKey Thank you!

It would be great if we first discussed this feature before its implementation 🙏
Because, I don't see any reasons to do recursive search for `.env` files.

I have o lot of projects with `.env` files (several files per project) and I haven't had situations where I need to use recursive search.

Other linters like `rubocop` can search recursively because projects written in Ruby can have subdirectories with `.rb` files.

To use `.env` files in our projects we should use `dotenv` libraries whick look for `.env` files in the current directory:
* Ruby: https://github.com/bkeepers/dotenv
* Rust: https://github.com/dotenv-rs/dotenv
* JS: https://github.com/motdotla/dotenv
* PHP: https://github.com/vlucas/phpdotenv
* Haskell: https://github.com/stackbuilders/dotenv-hs
* Python: https://github.com/theskumar/python-dotenv

Or we should use Docker Compose and keep `.env` files somewhere else (if we want it).
I use both of ways every day and don't need to use recursive search.
DDtKey: > @DDtKey Thank you!
> 
> It would be great if we first discussed this feature before its implementation pray
> Because, I don't see any reasons to do recursive search for `.env` files.
> 
> I have o lot of projects with `.env` files (several files per project) and I haven't had situations where I need to use recursive search.
> 
> Other linters like `rubocop` can search recursively because projects written in Ruby can have subdirectories with `.rb` files.
> 
> To use `.env` files in our projects we should use `dotenv` libraries whick look for `.env` files in the current directory:
> 
>     * Ruby: https://github.com/bkeepers/dotenv
> 
>     * Rust: https://github.com/dotenv-rs/dotenv
> 
>     * JS: https://github.com/motdotla/dotenv
> 
>     * PHP: https://github.com/vlucas/phpdotenv
> 
>     * Haskell: https://github.com/stackbuilders/dotenv-hs
> 
>     * Python: https://github.com/theskumar/python-dotenv
> 
> 
> Or we should use Docker Compose and keep `.env` files somewhere else (if we want it).
> I use both of ways every day and don't need to use recursive search.

Yes, my fault :)

But as I wrote above, **loading variables is not exactly the same as a linter**.

In my memory, there were cases when folders were grouped and had other files nested. And they were directly loaded by applications according to completely different scenarios.

For example, take `clippy`, because we can write many modules, and it will check everything, regardless of usage. Most linter do so.

In the end, I just do not see a problem in the presence of this function, it allows you to do the same thing, but also complements the feature :thinking: 
DDtKey: If this seems like an excess, okay, I will close the PR.
Thanks for the review!
mgrachev:  > Yes, my fault :)
> 
> But as I wrote above, **loading variables is not exactly the same as a linter**.
> 
> In my memory, there were cases when folders were grouped and had other files nested. And they were directly loaded by applications according to completely different scenarios.

Usually all `.env` files are kept in the root directory of projects or in another directory.
Right now we can check these files with the command:
```sh
$ dotenv-linter ./another-directory .
```

Do you know projects where `.env` files are stored differently?
It would be nice if you gave an example.

> For example, take `clippy`, because we can write many modules, and it will check everything, regardless of usage. Most linter do so.

I think, `clippy` is a bad example, because it works with projects written in Rust. In Rust projects we can have such structure:
```sh
├── src
│   ├── checks
│   │   ├── duplicated_key.rs
│   │   ├── ending_blank_line.rs
│   │   ├── extra_blank_line.rs
│   │   ├── incorrect_delimiter.rs
│   │   ├── key_without_value.rs
│   │   ├── leading_character.rs
│   │   ├── lowercase_key.rs
│   │   ├── quote_character.rs
│   │   ├── space_character.rs
│   │   ├── trailing_whitespace.rs
│   │   └── unordered_key.rs
│   ├── checks.rs
│   ├── common.rs
│   ├── fs_utils.rs
│   ├── lib.rs
│   └── main.rs
``` 

But, I haven't seen any projects with similar structure with `.env` files:
```sh
├── example.env
├── global.env
├── subdir1
│   ├── common.env
│   └── subdir3
│       └── sample.env
└── subdir2
    └── test.env
```

> In the end, I just do not see a problem in the presence of this function, it allows you to do the same thing, but also complements the feature 🤔

I see some problems:
1. Recursive search is slower than search in only one directory;
2. I know a lot of projects that store their dependecies in some directories (`vendor`, `node_modules`) and after recursive search we may get some warnings from dependecies or their dependecies.

mgrachev: > If this seems like an excess, okay, I will close the PR.
> Thanks for the review!

I think we can add this feature, but only as an additional option.

@mstruebing What do you think about it?
DDtKey: I really saw projects with the following structure :
```sh
environments
├── config.env
├── module1
│   ├── module.env
└── module2
    └── module2.env
```
But with a deep level of nesting, probably not, I have not seen.

I'm not saying that this is a best practice. But this happens and maybe someone is useful.
DDtKey: >     2. I know a lot of projects that store their dependecies in some directories (`vendor`, `node_modules`) and after recursive search we may get some warnings from dependecies or their dependecies.

Such cases can always be handled with exclude (it works well in the current PR), but yes, we can to turn off recursion by default.
wesleimp: In my experience, like @DDtKey, I have seen many projects with several `.env` files, mainly when monorepo (e.g. `lerna`) is used for frontend. I think this function is very useful and is nice to have this feature.

About the implementation, when recursion is used, will there be any default exclude list (`node_modules`, `vendor`, etc)?
mstruebing: I think adding it as an optional feature is okay.
I currently work in a monorepo for example where every sub-package hat it's own `.env`-file like @DDtKey already mentioned, for that case it would be useful.
But mostly it's not an issue as most projects have a single `.env` file at root level.

So yeah, I agree: optional feature :+1: 
DDtKey: @mgrachev Before doing any further actions for an optional feature, I would like to know your opinion.
If this is still useful, I will definitely make a change :relaxed:
mgrachev: I agree with @mstruebing. Please, add an additional flag to run recursive search.
DDtKey: > I agree with @mstruebing. Please, add an additional flag to run recursive search.

Added a flag, updated tests and documentation.
Recursive search only works when a flag is specified.

Glad to discuss possible codebase improvements :relaxed: 
mgrachev: @mstruebing Please, take a look at these changes 👀

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 18ed7a3..bb97a59 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@ # Changelog
 
 ## [Unreleased]
 ### 🚀 Added
+- Add flag to enable recursive search for `.env` files [#223](https://github.com/dotenv-linter/dotenv-linter/pull/223) ([@DDtKey](https://github.com/DDtKey))
 - Add docs [#226](https://github.com/dotenv-linter/dotenv-linter/pull/226) ([@wesleimp](https://github.com/wesleimp))
 - Add Windows publishing to release workflow [#211](https://github.com/dotenv-linter/dotenv-linter/pull/211) ([@DDtKey](https://github.com/DDtKey))
 - Add support canonicalize path for Windows [#213](https://github.com/dotenv-linter/dotenv-linter/pull/213) ([@DDtKey](https://github.com/DDtKey))
diff --git a/README.md b/README.md
index 40e7da9..1c96794 100644
--- a/README.md
+++ b/README.md
@@ -169,7 +169,7 @@ ## 🚀 Usage
 dir2/.my-env-file:1 LowercaseKey: The bar key should be in uppercase
 ```
 
-If you need to exclude a file from check, you can use the argument `--exclude FILE_PATH` or its short version `-e FILE_PATH`:
+If you need to exclude a file or directory from check, you can use the argument `--exclude PATH` or its short version `-e PATH`:
 
 ```shell script
 $ dotenv-linter --exclude .env.test
@@ -177,6 +177,14 @@ ## 🚀 Usage
 .env:3 UnorderedKey: The BAR key should go before the FOO key
 ```
 
+If you need a recursive `.env` file search inside directories, you can use the flag `--recursive` or its short version `-r`:
+
+```shell script
+$ dotenv-linter --recursive
+dir1/.env:2 DuplicatedKey: The FOO key is duplicated
+dir2/subdir/.env:3 IncorrectDelimiter: The FOO-BAR key has incorrect delimiter
+```
+
 If you need to skip some checks, you can use the argument `--skip CHECK_NAME` or its short version `-s CHECK_NAME`:
 
 ```shell script
@@ -184,7 +192,7 @@ ## 🚀 Usage
 .env:2 DuplicatedKey: The FOO key is duplicated
 ```
 
-If you need to view all available checks, you can use the argument `--show-checks`:
+If you need to view all available checks, you can use the flag `--show-checks`:
 
 ```shell script
 $ dotenv-linter --show-checks
diff --git a/docs/quick_start.md b/docs/quick_start.md
index 002f7bb..99063c5 100644
--- a/docs/quick_start.md
+++ b/docs/quick_start.md
@@ -29,6 +29,14 @@ ## 🚀 Quick Start
 .env:3 UnorderedKey: The BAR key should go before the FOO key
 ```
 
+If you need a recursive search inside directories (deeper than 1 level), you can use the flag `--recursive` or its short version `-r`:
+
+```shell script
+$ dotenv-linter --recursive
+dir1/.env:2 DuplicatedKey: The FOO key is duplicated
+dir2/subdir/.env:3 IncorrectDelimiter: The FOO-BAR key has incorrect delimiter
+```
+
 If you need to skip some checks, you can use the argument `--skip CHECK_NAME` or its short version `-s CHECK_NAME`:
 
 ```sh
diff --git a/src/lib.rs b/src/lib.rs
index ade314f..4ba3b54 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -11,51 +11,30 @@
 
 #[allow(clippy::redundant_closure)]
 pub fn run(args: clap::ArgMatches, current_dir: &PathBuf) -> Result<Vec<Warning>, Box<dyn Error>> {
-    let mut dirs: Vec<PathBuf> = Vec::new();
     let mut file_paths: Vec<PathBuf> = Vec::new();
     let mut skip_checks: Vec<&str> = Vec::new();
+    let mut excluded_paths: Vec<PathBuf> = Vec::new();
+
+    let is_recursive = args.is_present("recursive");
 
     if let Some(skip) = args.values_of("skip") {
         skip_checks = skip.collect();
     }
 
-    if let Some(inputs) = args.values_of("input") {
-        dirs = inputs
-            .clone()
-            .filter_map(|s| fs_utils::canonicalize(s).ok())
-            .filter(|p| p.is_dir())
-            .collect();
-
-        file_paths = inputs
-            .filter_map(|s| fs_utils::canonicalize(s).ok())
-            .filter(|p| p.is_file())
-            .collect();
-    }
-
-    for dir_path in dirs {
-        let entries = dir_path.read_dir()?;
-
-        let mut dir_files: Vec<PathBuf> = entries
-            .filter_map(|e| e.ok())
-            .map(|e| e.path())
-            .filter(|path| FileEntry::is_env_file(path))
+    if let Some(excluded) = args.values_of("exclude") {
+        excluded_paths = excluded
+            .filter_map(|f| fs_utils::canonicalize(f).ok())
             .collect();
-
-        file_paths.append(dir_files.as_mut());
     }
 
-    // Removes files from paths if they should be excluded
-    if let Some(excluded) = args.values_of("exclude") {
-        let excluded_paths: Vec<PathBuf> = excluded
-            .filter_map(|f| fs_utils::canonicalize(f).ok())
+    if let Some(inputs) = args.values_of("input") {
+        let input_paths = inputs
+            .filter_map(|s| fs_utils::canonicalize(s).ok())
             .collect();
 
-        file_paths.retain(|path| !excluded_paths.contains(&path));
+        file_paths.extend(get_file_paths(input_paths, &excluded_paths, is_recursive));
     }
 
-    file_paths.sort();
-    file_paths.dedup();
-
     let mut warnings: Vec<Warning> = Vec::new();
 
     for path in file_paths {
@@ -78,6 +57,38 @@ pub fn run(args: clap::ArgMatches, current_dir: &PathBuf) -> Result<Vec<Warning>
     Ok(warnings)
 }
 
+fn get_file_paths(
+    dir_entries: Vec<PathBuf>,
+    excludes: &[PathBuf],
+    is_recursive: bool,
+) -> Vec<PathBuf> {
+    let nested_paths: Vec<PathBuf> = dir_entries
+        .iter()
+        .filter(|entry| entry.is_dir())
+        .filter(|entry| !excludes.contains(entry))
+        .filter_map(|dir| dir.read_dir().ok())
+        .map(|read_dir| {
+            read_dir
+                .filter_map(|e| e.ok())
+                .map(|e| e.path())
+                .filter(|path| FileEntry::is_env_file(path) || (is_recursive && path.is_dir()))
+                .collect()
+        })
+        .flat_map(|dir_entries| get_file_paths(dir_entries, excludes, is_recursive))
+        .collect();
+
+    let mut file_paths: Vec<PathBuf> = dir_entries
+        .into_iter()
+        .filter(|entry| entry.is_file())
+        .filter(|entry| !excludes.contains(entry))
+        .collect();
+
+    file_paths.sort();
+    file_paths.extend(nested_paths);
+    file_paths.dedup();
+    file_paths
+}
+
 fn get_line_entries(fe: FileEntry, lines: Vec<String>) -> Vec<LineEntry> {
     let mut entries: Vec<LineEntry> = Vec::with_capacity(fe.total_lines);
 
diff --git a/src/main.rs b/src/main.rs
index 1294ed7..1ada611 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -60,5 +60,11 @@ fn get_args(current_dir: &OsStr) -> clap::ArgMatches {
                 .long("show-checks")
                 .help("Shows list of available checks"),
         )
+        .arg(
+            Arg::with_name("recursive")
+                .short("r")
+                .long("recursive")
+                .help("Recursively search and check .env files"),
+        )
         .get_matches()
 }


Test patch:
diff --git a/tests/args/specific_path.rs b/tests/args/specific_path.rs
index 08511d7..f7ae22d 100644
--- a/tests/args/specific_path.rs
+++ b/tests/args/specific_path.rs
@@ -93,11 +93,11 @@ fn checks_one_specific_file_and_one_path() {
 
     let args = &[testfile_2.as_str(), subdir.as_str()];
     let expected_output = format!(
-        "{}:2 DuplicatedKey: The FOO key is duplicated\n{}:2 UnorderedKey: The BAR key should go before the FOO key\n",
+        "{}:2 UnorderedKey: The BAR key should go before the FOO key\n{}:2 DuplicatedKey: The FOO key is duplicated\n",
+        testfile_2.shortname_as_str(),
         Path::new(&testdir.relative_path(&subdir))
             .join(testfile_3.shortname_as_str())
             .to_str().expect("multi-platform path to test .env file"),
-        testfile_2.shortname_as_str(),
     );
 
     testdir.test_command_fail_with_args(args, expected_output);
diff --git a/tests/options/mod.rs b/tests/options/mod.rs
index 364d24f..52a2eb6 100644
--- a/tests/options/mod.rs
+++ b/tests/options/mod.rs
@@ -1,1 +1,2 @@
 mod exclude;
+mod recursive;
diff --git a/tests/options/recursive.rs b/tests/options/recursive.rs
new file mode 100644
index 0000000..7043583
--- /dev/null
+++ b/tests/options/recursive.rs
@@ -0,0 +1,82 @@
+use crate::common::TestDir;
+use std::path::Path;
+
+#[test]
+fn checks_one_in_subdir() {
+    let test_dir = TestDir::new();
+    test_dir.create_testfile("correct.env", "FOO=BAR\n");
+    let test_subdir = test_dir.subdir();
+    let testfile_2 = test_subdir.create_testfile(".incorrect.env", "1BAR=\n");
+
+    let args = &["-r"];
+    let expected_output = format!(
+        "{}:1 LeadingCharacter: Invalid leading character detected\n",
+        Path::new(&test_dir.relative_path(&test_subdir))
+            .join(testfile_2.shortname_as_str())
+            .to_str()
+            .expect("multi-platform path to test .env file")
+    );
+
+    test_dir.test_command_fail_with_args(args, expected_output);
+}
+
+#[test]
+fn checks_files_in_deep_subdirs() {
+    let test_dir = TestDir::new();
+    test_dir.create_testfile("correct.env", "FOO=BAR\n");
+
+    let test_subdir_2 = test_dir.subdir();
+    let testfile_2 = test_subdir_2.create_testfile("incorrect.sub_1.env", "FOO=BAR\nBAR=FOO\n");
+
+    let test_subdir_3 = test_subdir_2.subdir();
+    let testfile_3 = test_subdir_3.create_testfile(".incorrect.env", "FOO=");
+
+    let args = &["--recursive"];
+    let expected_output = format!(
+        "{}:2 UnorderedKey: The BAR key should go before the FOO key\n{}:1 EndingBlankLine: No blank line at the end of the file\n",
+        Path::new(&test_dir.relative_path(&test_subdir_2))
+            .join(testfile_2.shortname_as_str())
+            .to_str()
+            .expect("multi-platform path to test .env file"),
+        Path::new(&test_dir.relative_path(&test_subdir_3))
+            .join(testfile_3.shortname_as_str())
+            .to_str()
+            .expect("multi-platform path to test .env file")
+    );
+
+    test_dir.test_command_fail_with_args(args, expected_output);
+}
+
+#[test]
+fn checks_without_recursive_flag() {
+    let test_dir = TestDir::new();
+    test_dir.create_testfile("correct.env", "FOO=BAR\n");
+    let test_subdir = test_dir.subdir();
+    test_subdir.create_testfile(".incorrect.env", "1BAR=\n");
+
+    // incorrect file located in a subdirectory should not be checked
+    test_dir.test_command_success();
+}
+
+#[test]
+fn checks_recursive_with_exclude_subdir() {
+    let test_dir = TestDir::new();
+    test_dir.create_testfile("correct.env", "FOO=BAR\n");
+
+    let test_subdir_2 = test_dir.subdir();
+    let testfile_2 = test_subdir_2.create_testfile("incorrect.sub_1.env", "FOO=BAR\nBAR=FOO\n");
+
+    let test_subdir_3 = test_subdir_2.subdir();
+    let testfile_to_exclude = test_subdir_3.create_testfile(".incorrect.env", "FOO=");
+
+    let args = &["--exclude", testfile_to_exclude.as_str(), "--recursive"];
+    let expected_output = format!(
+        "{}:2 UnorderedKey: The BAR key should go before the FOO key\n",
+        Path::new(&test_dir.relative_path(&test_subdir_2))
+            .join(testfile_2.shortname_as_str())
+            .to_str()
+            .expect("multi-platform path to test .env file"),
+    );
+
+    test_dir.test_command_fail_with_args(args, expected_output);
+}