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

Color output and --no-color option #307

Merged
merged 29 commits into from Nov 7, 2020

Conversation

srinikhil-07
Copy link
Contributor

@srinikhil-07 srinikhil-07 commented Oct 5, 2020

This commit if included will:

  1. Adds color to warnings,
  2. Adds a --no-color option to switch off colored output,
  3. Adds a test case to cover --no-color option

✔ 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).

This commit if included will:

1. Adds color to warnings,
2. Adds a --no-color option to switch off colored output,
3. Adds a test case to cover --no-color option
@srinikhil-07
Copy link
Contributor Author

@mgrachev

This PR is for #291.Please review. I have gone through #240 for coming up with this changes.

@mgrachev mgrachev linked an issue Oct 5, 2020 that may be closed by this pull request
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.

@Nikhil0487 Thank you for your contribution! 🔥

I have left some comments. Please look at them 👀

src/common/warning.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@srinikhil-07
Copy link
Contributor Author

srinikhil-07 commented Oct 6, 2020

@mgrachev Suggested changes added. Please verify

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
tests/options/no_color.rs Outdated Show resolved Hide resolved
@mgrachev mgrachev requested a review from a team October 6, 2020 08:53
@srinikhil-07
Copy link
Contributor Author

@mgrachev

While pulling master, I get already up to date. But after pushing I get a conflict.

tests/common/output.rs Outdated Show resolved Hide resolved
src/common/warning.rs Outdated Show resolved Hide resolved
src/common/warning.rs Outdated Show resolved Hide resolved
src/common/warning.rs Outdated Show resolved Hide resolved
tests/options/no_color.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@mgrachev
Copy link
Member

mgrachev commented Oct 6, 2020

I have noticed the issue colored-rs/colored#59 that it doesn't wotk on Windows.
@Nikhil0487 @DDtKey Could you check it?

@codecov-io
Copy link

codecov-io commented Oct 6, 2020

Codecov Report

Merging #307 into master will decrease coverage by 0.02%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
- Coverage   97.07%   97.05%   -0.03%     
==========================================
  Files          33       33              
  Lines        2363     2379      +16     
==========================================
+ Hits         2294     2309      +15     
- Misses         69       70       +1     
Impacted Files Coverage Δ
src/common.rs 100.00% <ø> (ø)
src/main.rs 91.52% <88.88%> (-0.79%) ⬇️
src/common/output.rs 96.07% <100.00%> (+0.33%) ⬆️
src/common/warning.rs 90.32% <100.00%> (+1.86%) ⬆️

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 8d38055...e24e487. Read the comment docs.

@srinikhil-07
Copy link
Contributor Author

@mgrachev I have gone through the issue colored-rs/colored#59. Seems this can be fixed by enabling virtual terminal flag. I have added it but I have no Windows machine to check it manually.

@srinikhil-07
Copy link
Contributor Author

@mgrachev will check other suggested changes.

This commit if included will:

1. Change file line and number to italics,
2. Code review changes to common_output function,
3. Test cases changes for points 1, 2
@srinikhil-07
Copy link
Contributor Author

@mgrachev Suggested changes except --no-color bug, and a way to test --no-color option pushed.

Will figure out changes w.r.t --no-color.

@mgrachev mgrachev requested a review from DDtKey October 20, 2020 19:44
@srinikhil-07
Copy link
Contributor Author

@mgrachev @DDtKey
The cfg Windows review comment has also been resolved. Please check if i got it right, will change if not.
Also, the --no-color test has been removed.

@mgrachev mgrachev added hacktoberfest-accepted Hacktoberfest accepted wait Wait for something labels Oct 21, 2020
@mgrachev mgrachev added this to the v3.0.0 milestone Oct 21, 2020
@mgrachev
Copy link
Member

@Nikhil0487 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.

Thanks for your PR! 👍

I will also try to return to the topic of testing. But at the moment I have no complete ideas 🤔

src/main.rs Outdated Show resolved Hide resolved
@mgrachev mgrachev mentioned this pull request Oct 26, 2020
16 tasks
DDtKey
DDtKey previously approved these changes Oct 26, 2020
@mgrachev
Copy link
Member

mgrachev commented Nov 2, 2020

@Nikhil0487 Please resolve conflicts.

@srinikhil-07
Copy link
Contributor Author

srinikhil-07 commented Nov 3, 2020

@mgrachev

I have fixed some tests and resolved conflicts. Please review.

Also, I have colored "No problems found" text to green. Is that ok.

@srinikhil-07
Copy link
Contributor Author

srinikhil-07 commented Nov 3, 2020

@mgrachev @DDtKey

Since we're coloring texts, can we consider the following:

  1. Can we write the word "Checking" in some light color (like grey/purple etc.) and "file.env" part in some dark color in "Checking file.env" line or the whole text in same light color ,
  2. Can we color texts "All warnings are fixed." in bold green and "Total:" in some other bold color or with same bold green,
  3. Can we color "No warnings found" and "No problems found" in bold green,
  4. We have "QuoteCharacter" (example problem) text and "Found n problems" text in same red bold color. Can we have different yet bold colors for these two to show distinctiveness.
  5. Example text: 'Original file was backed up to: "test.env_1604375570"'. Can we follow something similar to point 3.

That is in short since we're applying colors anyway, can we audit coloring for all texts and make it more colorful?

@srinikhil-07
Copy link
Contributor Author

@mgrachev @DDtKey

Another interesting thing i observed is that for tests in src/common/output.rs, I had to apply colors to pass tests.
Can we somehow apply the fmt::Display trait to our test suite framework and check colors?

@mgrachev
Copy link
Member

mgrachev commented Nov 7, 2020

Since we're coloring texts, can we consider the following:

  1. Can we write the word "Checking" in some light color (like grey/purple etc.) and "file.env" part in some dark color in "Checking file.env" line or the whole text in same light color ,
  2. Can we color texts "All warnings are fixed." in bold green and "Total:" in some other bold color or with same bold green,
  3. Can we color "No warnings found" and "No problems found" in bold green,
  4. We have "QuoteCharacter" (example problem) text and "Found n problems" text in same red bold color. Can we have different yet bold colors for these two to show distinctiveness.
  5. Example text: 'Original file was backed up to: "test.env_1604375570"'. Can we follow something similar to point 3.

That is in short since we're applying colors anyway, can we audit coloring for all texts and make it more colorful?

I think we can do it a bit later. Please open issues for each items with screenshots and we will discuss them.

@mgrachev
Copy link
Member

mgrachev commented Nov 7, 2020

Another interesting thing i observed is that for tests in src/common/output.rs, I had to apply colors to pass tests.
Can we somehow apply the fmt::Display trait to our test suite framework and check colors?

I don't have any ideas right now. Maybe @dotenv-linter/core you have?

@mgrachev mgrachev merged commit 6769474 into dotenv-linter:master Nov 7, 2020
@mgrachev
Copy link
Member

mgrachev commented Nov 7, 2020

@Nikhil0487 Awesome work 🔥 Thank you! 👍

Please add information about this feature to dotenv-linter.github.io.

@srinikhil-07 srinikhil-07 deleted the dotenv-color branch November 8, 2020 11:58
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.

Add colored output and --no-color argument
5 participants