fix #43: pub use crates, add set_log_verbosity and feature=full-throttle by vitiral · Pull Request #51 · killercup/quicli

vitiral · 2018-02-10T16:15:48Z

This does several things to, some of which is to integrate better with the ergo ecosystem.

Adds feature=full-throttle as suggested in Integrate with the ergo ecosystem #43, which allows disabling some of the included crates
Moves set_log_verbosity into a function so that users can more cleanly migrate away from the main! macro (I already had need of this in artifact).
Publically uses crates to improve the import story. For instance, to use clap (which is exported by quicli), I had to do: use quicli::prelude::structopt::clap, which now I can do use quicli::structopt::clap.

I am currently using both ergo and quicli together to rewrite artifact's CLI. I'll keep opening bugs/PRs as I find them!

vitiral · 2018-02-10T16:23:26Z

just a sec, something isn't right -- I can't use derive(StructOpt) anymore...

vitiral · 2018-02-10T16:29:58Z

nevermind, all is well again (I knew there was a merge conflict! Thanks for adding that test)

…ull-throttle

killercup

I am currently using both ergo and quicli together to rewrite artifact's CLI. I'll keep opening bugs/PRs as I find them!

Cool!

Publically uses crates to improve the import story.

Not sure this is a good idea. If you are honestly using clap in your crate for anything, you should probably add it to your Cargo.toml and use the same version as quicli. That's more obvious to any contributors including your future self. I'd be interested in what other think about this.

killercup · 2018-02-10T16:33:54Z

Cargo.toml

+
+[dependencies.serde]
+version = "1.0.27"
+optional = true


Can you convert all these to inline-table style ? Like this: serde = { version = "1", optional = true }

sure, but anytime you do a cargo add they will be converted back 😄

have you heard the good news of cargo install cargo-edit --vers 0.3.0-beta.1 -f? :D

what? No! Does it preserve formatting?!?!

It should! Please tell me if it breaks!

praise the rust godess, it works!

killercup · 2018-02-10T16:36:24Z

src/easy_log.rs

+/// This is used in the [`main!`] macro. You should typically use that instead.
+///
+/// [`main!`]: macro.main.html
+pub fn set_log_verbosity(verbosity: u64) -> prelude::Result<()> {


If we do expose this, we should probably also make it parametrized over package_name (currently Some(env!("CARGO_PKG_NAME"))) and default_level. I have more thoughts on this but wrote them in #46 (comment)

I like that.

I'm not sure what default_level is. Is that an argument? How would it be used? I currently like the behavior that verbosity=0 gives you the default of error logging. If you don't want any logging than just don't call set_log_verbosity!

Actually, I retract that comment. Let's either

change this to a thing that does the mapping from int to log level (basically return log_level and skip the rest)

or to a more complicated thing with a builder like Logger::new().this_crate(2).other_crates("Warn").init()? (note it's generic over ints and strings) and go all in, with the option of adding more cool stuff later on (e.g. detect that this is run without a tty and write to syslog/journald)

The former is a bit conservative and requires you to write the LoggerBuilder call with the two filters yourself. It also gives you more flexibility that way, of course. The latter is way more complicated, but might allow using quicli in more contexts.

FYI, I haven't seen an easy logging library yet that gives us more than env_logger. Most others require setting up a bunch of different adaptors.

I actually think env_logger is the right choice for this library. I didn't realize it lets you control the logging through both a specified verbosity and env-vars.

Hmm... I think that is providing too much complexity. What if there was a other_verbosity: Option<u64> which by default would do the behavior you defined in #46, but would allow you to do something like other_verbosity: Some(u64::min(cmd.verbosity, 2)).

I think kicking off the logging is a huge benefit here. I don't think quicli is the right place to solve all logging ergonomics.

killercup · 2018-02-10T16:40:31Z

src/lib.rs

-extern crate serde;
+#[cfg(feature="full-throttle")]
+#[macro_use]
+pub extern crate serde_derive;


I'm not sure I agree with making these dependencies public. Let me think about this for a bit.

Definitely. I think I'll remove it from this review and open an issue for discussion.

vitiral · 2018-02-10T16:57:27Z

Not sure this is a good idea. If you are honestly using clap in your crate for anything, you should probably add it to your Cargo.toml and use the same version as quicli.

I don't think I agree. clap is exported from structopt for a reason -- structopt is just a wrapper. quicli (and ergo) are just "ecosystem conglomeration and wrapping crates". It is true that we try to provide a clean "interface", but sometimes you need the "wrapped type". It's a maintenance burden to require the user to specify the version of clap they want when all they really want is "the same version as structopt uses".

I'd definetly be interested in more thoughts/opinions as ergo is taking the position of always using pub extern crate to uphold this contract.

vitiral · 2018-02-10T17:08:12Z

I opened #52 for the pub extern crate issue. I'll remove those changes as well.

vitiral · 2018-02-10T17:26:31Z

Okay, I think the only outstanding issue is the exported log function.

killercup · 2018-02-11T07:45:40Z

Thanks for the quick changes! I'll release 0.2 with structopt 0.2 in a bit, this will make it into 0.2.1.

vitiral · 2018-02-11T08:54:14Z

So I'm thinking the most flexibility could be gained by an API like:

fn set_log_verbosity(lowest: u64, levels: &[(&str, u64)])

lowest is the lowest log level, i.e. set to 1 to enable all warnings.
levels is a tuple containing the custom levels for specific crates.

This gives complete flexibility but is also very simple.

vitiral · 2018-02-12T22:44:43Z

@killercup what do you think about the new API?

vitiral · 2018-02-13T16:12:50Z

I'm going to make the logging stuff a separate issue.

Edit: #56

vitiral · 2018-02-13T16:24:39Z

Okay, I made set_log_verbosity hidden so it is now just an implementation detail of the current main! macro. Hopefully that works for you!

…ate on it

killercup · 2018-02-16T15:53:55Z

Thanks for the updates, @vitiral! And sorry for taking to long to get back to you. I'm currently dealing with some other things, but hope to get around to reviewing this over the weekend.

killercup · 2018-02-16T15:55:20Z

Gotta be honest, though, a PR that just adds the feature flag would've been merged for a while now ;)

vitiral · 2018-02-16T16:20:28Z

Ya, keep changes small :) Most of this was an experiment to show what was needed to integrate with ergo. I'm happy to take a bit longer to figure that out. I think the issues opened for further discussion were worth the friction, and this showed to me that we should be able to integrate cleanly eventually.

vitiral · 2018-03-12T23:32:19Z

going to reopen this as a separate PR for just the feature flag.

vitiral force-pushed the no-features branch from 2ceb965 to 052a78e Compare February 10, 2018 16:29

fix killercup#43: pub use crates, add set_log_verbosity and feature=f…

052a78e

…ull-throttle

killercup suggested changes Feb 10, 2018

View reviewed changes

vitiral mentioned this pull request Feb 10, 2018

"pub extern crate" for all dependencies #52

Open

improve Cargo.toml format

4bf8a18

remove pub from extern

41693e3

vitiral mentioned this pull request Feb 13, 2018

Add set_log_verbosity function #56

Open

killercup#56: hide the set_log_verbosity function so that we can iter…

711bbaf

…ate on it

vitiral closed this Mar 12, 2018

vitiral mentioned this pull request Mar 12, 2018

add full-throttle feature #70

Merged

Conversation

vitiral commented Feb 10, 2018

Uh oh!

vitiral commented Feb 10, 2018

Uh oh!

vitiral commented Feb 10, 2018

Uh oh!

killercup left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitiral Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitiral commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitiral commented Feb 10, 2018

Uh oh!

vitiral commented Feb 10, 2018

Uh oh!

killercup commented Feb 11, 2018

Uh oh!

vitiral commented Feb 11, 2018

Uh oh!

vitiral commented Feb 12, 2018

Uh oh!

vitiral commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitiral commented Feb 13, 2018

Uh oh!

killercup commented Feb 16, 2018

Uh oh!

killercup commented Feb 16, 2018

Uh oh!

vitiral commented Feb 16, 2018 via email

Uh oh!

vitiral commented Mar 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vitiral Feb 10, 2018 •

edited

Loading

vitiral commented Feb 10, 2018 •

edited

Loading

vitiral commented Feb 13, 2018 •

edited

Loading