Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shows files that were linted #311

Merged
merged 7 commits into from Oct 26, 2020
Merged

Conversation

Anthuang
Copy link
Contributor

@Anthuang Anthuang commented Oct 6, 2020

This PR adds functionality to show the files that were linted as part of a dotenv-linter run. It does so by introducing a new struct Warnings which is basically a wrapper around Vec<Warning> to also include information about the paths related to the warnings.

This PR also fixes tests as a result of the added functionality, and some refactoring.

Below shows the output from running dotenv-linter with different args:

$ cargo run
Checking ".env"
.env:1 LowercaseKey: The hi key should be in uppercase
.env:3 ExtraBlankLine: Extra blank line detected
.env:4 LowercaseKey: The hi3 key should be in uppercase

Checking ".env_2/.env"
.env_2/.env:2 ExtraBlankLine: Extra blank line detected
.env_2/.env:3 LowercaseKey: The hi key should be in uppercase
.env_2/.env:4 DuplicatedKey: The hi key is duplicated
.env_2/.env:4 LowercaseKey: The hi key should be in uppercase

Found 7 problems
$ cargo run -- -q
.env:1 LowercaseKey: The hi key should be in uppercase
.env:3 ExtraBlankLine: Extra blank line detected
.env:4 LowercaseKey: The hi3 key should be in uppercase
.env_2/.env:2 ExtraBlankLine: Extra blank line detected
.env_2/.env:3 LowercaseKey: The hi key should be in uppercase
.env_2/.env:4 DuplicatedKey: The hi key is duplicated
.env_2/.env:4 LowercaseKey: The hi key should be in uppercase
$ cargo run -- -f
Checking ".env"
Original file was backed up to: ".env_1601943961"

.env:1 LowercaseKey: The hi key should be in uppercase
.env:3 ExtraBlankLine: Extra blank line detected
.env:4 LowercaseKey: The hi3 key should be in uppercase

Checking ".env_2/.env"
Original file was backed up to: ".env_2/.env_1601943961"

.env_2/.env:2 ExtraBlankLine: Extra blank line detected
.env_2/.env:3 LowercaseKey: The hi key should be in uppercase
.env_2/.env:4 DuplicatedKey: The hi key is duplicated
.env_2/.env:4 LowercaseKey: The hi key should be in uppercase

All warnings are fixed. Total: 7
$ cargo run -- -f -q
Original file was backed up to: ".env_1601943979"
Original file was backed up to: ".env_2/.env_1601943979"

All warnings are fixed. Total: 7
$ cargo run -- -f --no-backup
Checking ".env"
.env:1 LowercaseKey: The hi key should be in uppercase
.env:3 ExtraBlankLine: Extra blank line detected
.env:4 LowercaseKey: The hi3 key should be in uppercase

Checking ".env_2/.env"
.env_2/.env:2 ExtraBlankLine: Extra blank line detected
.env_2/.env:3 LowercaseKey: The hi key should be in uppercase
.env_2/.env:4 DuplicatedKey: The hi key is duplicated
.env_2/.env:4 LowercaseKey: The hi key should be in uppercase

All warnings are fixed. Total: 7
$ cargo run -- -f -q --no-backup

All warnings are fixed. Total: 7

✔ Checklist:

  • This PR has been added to CHANGELOG.md (at the top of the list);
  • Tests for the changes have been added (for bug fixes / features);
  • Docs have been added / updated (for bug fixes / features).

Closes #268

@Anthuang
Copy link
Contributor Author

Anthuang commented Oct 6, 2020

I think the error is a bug from cargo itself?

@mgrachev
Copy link
Member

mgrachev commented Oct 6, 2020

@Anthuang Thank you for your contribution! 🔥

I think the error is a bug from cargo itself?

I think not 🤔

failures:

---- common::warnings::tests::warnings_fmt_test stdout ----
---- common::warnings::tests::warnings_fmt_test stderr ----
thread 'main' panicked at 'Could not create FileEntry', src/common/warnings.rs:71:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@mgrachev mgrachev requested a review from a team October 6, 2020 07:43
@mgrachev mgrachev added this to the v3.0.0 milestone Oct 6, 2020
@Anthuang
Copy link
Contributor Author

Anthuang commented Oct 6, 2020

@Anthuang Thank you for your contribution! 🔥

I think the error is a bug from cargo itself?

I think not 🤔

failures:

---- common::warnings::tests::warnings_fmt_test stdout ----
---- common::warnings::tests::warnings_fmt_test stderr ----
thread 'main' panicked at 'Could not create FileEntry', src/common/warnings.rs:71:21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I made the mistake of not scrolling up haha

Copy link
Member

@mgrachev mgrachev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Anthuang Thank you for your contribution! 🔥

I have left some comments. Please look at them 👀

src/common/warnings.rs Outdated Show resolved Hide resolved
src/common.rs Outdated Show resolved Hide resolved
src/common/warnings.rs Outdated Show resolved Hide resolved
src/common/warnings.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
tests/common/output.rs Outdated Show resolved Hide resolved
@mgrachev mgrachev requested a review from a team October 6, 2020 09:22
@Anthuang
Copy link
Contributor Author

Anthuang commented Oct 6, 2020

Thanks for the review!!

@codecov-io
Copy link

codecov-io commented Oct 7, 2020

Codecov Report

Merging #311 into master will decrease coverage by 0.29%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #311      +/-   ##
==========================================
- Coverage   96.98%   96.69%   -0.30%     
==========================================
  Files          32       33       +1     
  Lines        2292     2333      +41     
==========================================
+ Hits         2223     2256      +33     
- Misses         69       77       +8     
Impacted Files Coverage Δ
src/common.rs 100.00% <ø> (ø)
src/common/output.rs 75.75% <75.75%> (ø)
src/lib.rs 98.48% <100.00%> (-0.03%) ⬇️
src/main.rs 90.00% <100.00%> (+2.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b44f73...3b5e867. Read the comment docs.

src/common/output.rs Outdated Show resolved Hide resolved
src/common/output.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/common/output.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
tests/common/output.rs Outdated Show resolved Hide resolved
tests/common/output.rs Show resolved Hide resolved
tests/flags/quiet.rs Show resolved Hide resolved
@Anthuang
Copy link
Contributor Author

I messed up the git history a bit when rebasing, hopefully this fixes it

src/common.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
tests/common/output.rs Show resolved Hide resolved
tests/common/output.rs Show resolved Hide resolved
tests/fixes/correct_output.rs Outdated Show resolved Hide resolved
@mgrachev mgrachev requested review from a team, DDtKey and mstruebing and removed request for a team October 12, 2020 16:46
@mgrachev
Copy link
Member

@Anthuang Please fix conflicts.

@Anthuang
Copy link
Contributor Author

@Anthuang Please fix conflicts.

Done!

@mgrachev mgrachev added hacktoberfest-accepted Hacktoberfest accepted wait Wait for something labels Oct 21, 2020
@mgrachev
Copy link
Member

@Anthuang Thank you! I'm going to merge this PR after v2.2.1 release.

Copy link
Member

@DDtKey DDtKey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! 👍

To be honest, this is not exactly what I imagined after #268 🙁
It seems to me that it was about interactive mode.
When we see the progress of checking files on the fly (many CLI utilities do just that)

Perhaps the dotenv-linter works very quickly and even with a large number of files it is not necessary, but in this case, it generally does not make much sense, except for displaying the files that have been checked(which is also important) 🤔

What do you say about it @Anthuang @dotenv-linter/core @gillespiecd ?

Either way, moving some of the output logic into a separate module and output of checked files is a great idea! 🚀

src/lib.rs Outdated Show resolved Hide resolved
@mgrachev
Copy link
Member

It seems to me that it was about interactive mode.
When we see the progress of checking files on the fly (many CLI utilities do just that)

You are absolutely right. This is just the first iteration.
We can add the progress of checking files on the fly later.

Perhaps the dotenv-linter works very quickly and even with a large number of files it is not necessary, but in this case, it generally does not make much sense, except for displaying the files that have been checked(which is also important) 🤔

Right now dotenv-linter does not output anything if it did not find problems in the .env files.
In such situation, it is not obvious to the user whether dotenv-linter has checked all the files or not.
So, this PR fixes that case.

@DDtKey
Copy link
Member

DDtKey commented Oct 21, 2020

You are absolutely right. This is just the first iteration.
We can add the progress of checking files on the fly later.

I completely agree with the division of the task into iterations! 👍

Right now dotenv-linter does not output anything if it did not find problems in the .env files.
In such situation, it is not obvious to the user whether dotenv-linter has checked all the files or not.
So, this PR fixes that case.

Then it might be more correct at the moment to write Checked {} instead of Checking {}, since the output occurs after all the checks are completed?
But in general, it is not necessary that in the future the structure of the output does not change much with the appearance of the output "on the fly" 🙂

@mgrachev
Copy link
Member

But in general, it is not necessary that in the future the structure of the output does not change much with the appearance of the output "on the fly" 🙂

@DDtKey Maybe you would like to implement on-the-fly output? 😉

@DDtKey
Copy link
Member

DDtKey commented Oct 22, 2020

But in general, it is not necessary that in the future the structure of the output does not change much with the appearance of the output "on the fly" 🙂

@DDtKey Maybe you would like to implement on-the-fly output? 😉

Why not, I'll gladly take it when we merge to master 🙂

@mgrachev
Copy link
Member

@Anthuang Please fix linter's warnings.

@mgrachev mgrachev merged commit a5903f9 into dotenv-linter:master Oct 26, 2020
@mgrachev
Copy link
Member

@Anthuang Thanks a lot! 🔥

@Anthuang
Copy link
Contributor Author

Thanks for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Hacktoberfest accepted wait Wait for something
Development

Successfully merging this pull request may close these issues.

Display file names being scanned
4 participants