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

Send all progress logging messages to stderr. #171

Merged

Conversation

dmaahs2017
Copy link
Contributor

@dmaahs2017 dmaahs2017 commented Mar 26, 2021

Adresses #169.

  • Send logging messages to stderr instead of stdout so pagers do not intercept them.
  • Updated tests to match these changes.
  • Changed test_markdown_rendering to no longer rely on the tldr pages
    repo. Instead manually adding a known page to use in this test,
    because it failed due to the tldr pages repo updating this command (so
    it no longer matched our expected output, which shouldn't cause our
    tests to fail).

@niklasmohrin
Copy link
Collaborator

To keep this consistent with the rest of the program, messages like "Successfully deleted cache." should also be printed on stderr. Maybe even the "page not found" output, but I am not sure about that one. (@dbrgn ?)

@dmaahs2017 As you can see, the tests have to be updated accordingly, but that should be a quick fix

@dmaahs2017 dmaahs2017 changed the title --update success message to stderr instead of stdout. Pager not to be used for Progress messages Mar 26, 2021
@dmaahs2017 dmaahs2017 changed the title Pager not to be used for Progress messages Pager not to be used for Progress messages [Do Not Merge Yet] Mar 26, 2021
@dmaahs2017 dmaahs2017 changed the title Pager not to be used for Progress messages [Do Not Merge Yet] WIP: Pager not to be used for Progress messages [Do Not Merge Yet] Apr 14, 2021
Copy link
Collaborator

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

This is a good thing to do (no matter how we handle the paging issue). However, the tests need an update (expect the string in stderr, not stdout).

@dmaahs2017 can you update the tests as well?

 - updated tests to expect these messages in stderr instead of stdout

 - changed test_markdown_rendering to no longer rely on the tldr pages
   repo. Instead manually adding a known page to use in this test,
   because it failed due to the tldr pages repo updating this command (so
   it no longer matched our expected output, which shouldn't cause our
   tests to fail).
@dmaahs2017 dmaahs2017 changed the title WIP: Pager not to be used for Progress messages [Do Not Merge Yet] Send all logging messages to stderr, so pagers do not pick them up. May 10, 2021
@dmaahs2017 dmaahs2017 changed the title Send all logging messages to stderr, so pagers do not pick them up. Send all logging messages to stderr. May 10, 2021
@dmaahs2017 dmaahs2017 requested a review from dbrgn May 10, 2021 03:55
@dmaahs2017
Copy link
Contributor Author

dmaahs2017 commented May 10, 2021

Ready for re-review. @niklasmohrin @dbrgn.

I grepped the src files for all the println! statements and changed the ones that fell into the "progress logging" category to stderr. Left the ones that were the intended output of a command as stdout.

@dmaahs2017 dmaahs2017 changed the title Send all logging messages to stderr. Send all progress logging messages to stderr. May 10, 2021
.assert()
.success()
.stdout(contains("Successfully updated cache."));
testenv.add_entry("tar", include_str!("tar-markdown.expected"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏻

Copy link
Collaborator

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@dbrgn dbrgn merged commit 388deac into tealdeer-rs:master May 10, 2021
@dmaahs2017 dmaahs2017 deleted the bug/issue-169-update-output-to-stderr branch May 10, 2021 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants