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

Allow debug logs to work with clap tests themselves #1851

Merged
merged 1 commit into from
Apr 22, 2020
Merged

Allow debug logs to work with clap tests themselves #1851

merged 1 commit into from
Apr 22, 2020

Conversation

pksunkara
Copy link
Member

@pksunkara pksunkara commented Apr 22, 2020

No description provided.

@pksunkara pksunkara marked this pull request as draft April 22, 2020 16:35
@pksunkara
Copy link
Member Author

pksunkara commented Apr 22, 2020

Oops. Looks like specifying deps based on some cfg won't work

The same applies to cfg(debug_assertions), cfg(test) and cfg(proc_macro). These values will not work as expected and will always have the default value returned by rustc --print=cfg. There is currently no way to add dependencies based on these configuration values.

So, they will always be included. But they won't actually be used anywhere when debug_assertions is set false.

Related to rust-lang/cargo#5777

@pksunkara pksunkara marked this pull request as ready for review April 22, 2020 16:56
Copy link
Contributor

@CreepySkeleton CreepySkeleton left a comment

Choose a reason for hiding this comment

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

To be honest, I am a bit unhappy with this PR. More specifically:

  • clap is a library, not the binary. Libraries are not supposed to initialize the logger:

    From log crate' doc:

    Libraries should link only to the log crate, and use the provided macros to log whatever information will be useful to downstream consumers.

    We may use it as internal-only hack, but we would need to put it behind a cfg (and test with RUSTFLAGS='--cfg clap_debug' cargo run ...).

  • Output format: please don't strip struct' name from output. It really helps me to debug things, and module name is not always enough. Also, can we insert a tab between [...] and the message so the message appears aligned? Logs can be lengthy and it's not particularly easy to scan though it when it's unaligned.

clap_generate/src/lib.rs Outdated Show resolved Hide resolved
src/build/app/mod.rs Outdated Show resolved Hide resolved
src/build/app/mod.rs Show resolved Hide resolved
Copy link
Contributor

@CreepySkeleton CreepySkeleton left a comment

Choose a reason for hiding this comment

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

Great! Just a small nitpick

src/parse/parser.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@CreepySkeleton CreepySkeleton left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 22, 2020

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@pksunkara
Copy link
Member Author

bors r=CreepySkeleton

@bors
Copy link
Contributor

bors bot commented Apr 22, 2020

Build succeeded:

@bors bors bot merged commit 0293fd7 into master Apr 22, 2020
@bors bors bot deleted the log branch April 22, 2020 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants