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

Cli ergonomics #9

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@gibix
Contributor

gibix commented May 29, 2018

replaces #7
closes #8

@gibix gibix force-pushed the gibix:cli-ergonomics branch 2 times, most recently from 89bc110 to 22c3960 May 30, 2018

@cardoe

I really fail to see what switching to quicli gets me in this. Its the new kid on the block for option parsers and it will be broken in the Rust 2018 Edition due to some changes with errors coming up. I'm also not a fan of the macro style. Effectively this is what commit messages are for. To lay out your argument as to WHY this change should be made. WHY is quicli better than using iq-cli or structopt directly or even clap directly? The change to using quicli should be its own logical commit with a commit message explaining WHY its the right choice.

I also fail to see the advantage of switching to cargo-metadata over using cargo directly. In the past when there were changes like workspaces cargo-metadata was broken for some time and a project I had that used it needed to be redone so I'm hesitant to depend on it again. Same thing here with regards to the commit messages. The change to cargo-metadata should be its own logical commit with the reasoning of WHY this change should be included.

Overall this PR says is about ergonomics and you had mentioned on IRC that it was to remove boilerplate but ignoring the addition of the test in here its adding 775 lines of code and removing 599 so I fail to see how it achieves that.

I honestly don't see how this moves the needle towards making this easier with/for Gentoo.

@lu-zero

This comment has been minimized.

lu-zero commented May 31, 2018

Ideally I'd split the changes as well so we can merge what is non-controversial and discuss what's good about structopt vs quickcli or make sure the speed gained by switching with cargo-metadata is compelling since we have at least one person burnt by it :)

@gibix

This comment has been minimized.

Contributor

gibix commented May 31, 2018

quicli as now is used only to re-export structopt, logenv, etc... it simply reduces imports, if you think is bad I can just drop it.

Cargo-metadata is activelly maintained in the cargo project by the team, there are also plans to add new features to it: 1 2.

The PR removes boilerplate and the needs of have useless fuctions like resolve, workspace, etc... IMO is more clean now.

@gibix gibix force-pushed the gibix:cli-ergonomics branch from 22c3960 to 47ea712 Jun 5, 2018

@gibix

This comment has been minimized.

Contributor

gibix commented Jun 6, 2018

The pr has been reorganized with meaningful commits.

@lu-zero

This comment has been minimized.

lu-zero commented Jun 6, 2018

I'm pretty sure this project uses the following format for commit messages

Short subject line

More meaningful explanation spanning more than
a single line.
With possibly a full rationale for the patch

Could you please update them to use the same formats. Tools such as tig works in a much nicer way if you use this format.

gibix added some commits May 30, 2018

Travis updates
- add cargo cache to travis
- fix travis badge into cargo.toml
- 1.17.0 build can fail
Small code shake-up
Use rust modules to reflect the following code design:
- src/main.rs	cli, setup, logs, IO
- src/lib.rs	core logic (can be re-exported)
Bump cargo version to 1.27
The api breaks in different point, particularly there is no more a
`call_main_without_stdin` function.  Because of that the cli arguments
are temporarily dropped.
Use structopt for argument parsing
Structopt is the ergonomic argument-as-structures in rust. The arguments
are organized in commands and subcommands:

- src/main.rs	non-cargo configuration and cli parsing (-v, -q)
- src/lib.rs	real cli parsing and defaults (build [args], etc)
Better error handling with failure::Error
The main returns a Result<(), failure::Error> that wraps all the
possible errors from libraries.

@gibix gibix force-pushed the gibix:cli-ergonomics branch from 47ea712 to 2bfb520 Jun 6, 2018

@gibix

This comment has been minimized.

Contributor

gibix commented Jun 6, 2018

@lu-zero thanks for the review

@gibix gibix force-pushed the gibix:cli-ergonomics branch 2 times, most recently from 6fe5067 to add0f9e Jun 10, 2018

gibix added some commits Jun 5, 2018

Drop cargo for cargo_metadata
Cargo_metadata is a crate the expose the cargo-metadata subcommand. Rely
on metadata and not on cargo directly reduces considerably both the
runtime and compile-time costs.  The downside is that cargo_metadata
doesn't expose cargo's internal types and metadata. By now is not a
blocking issue because the missing informations can be extracted
directly from the manifest file.  Has been added also another argument
to the `build` to specify a manifest path, because of that now
cargo-ebuild doesn't rely anymore on a cargo project but only on the
manifest.  This change removes also some boilerplate code like resolve,
workspace, etc.

In future work should ported the [package.metadata] field of the
`Cargo.toml` to handle cargo-ebuild specific configurations (es
KEYWORDS).

In future new version of the cargo-metadata format and cargo features
should be mapped into cargo_metadata in order to be used.
Add first integration test
The simple test solution is on the ebuild generated for cargo-ebuild
itself. After editing cargo.toml or `cargo update` is necessary to
upadate the test in `test/integration.rs`.

@gibix gibix force-pushed the gibix:cli-ergonomics branch from add0f9e to 96d7e97 Jun 13, 2018

@cardoe

Overall you improved the commit going from cargo to cargo_metadata along with its commit message. But that then begs the question of why there's a commit changing the cargo version if the series ultimately drops cargo in the end. Drop that patch. A number of the earlier patches need refinement still.

@@ -9,6 +10,7 @@ rust:
matrix:
allow_failures:
- rust: nightly
- rust: 1.17.0

This comment has been minimized.

@cardoe

cardoe Jun 13, 2018

Owner

I don't see justification in the commit message why this should be allowed to fail. The commit message is not a place to state what has changed but a place to rationalize your changes. So why can it fail? I don't see anything in this commit that would lead me to believe WHY this should be allowed to fail. There are two realities:

  • in a later commit it starts failing because the minimum supported version needs to be changed so this change belongs in that commit
  • some other reason like one test fails intermittently with a later change and this change should be with that code change

This comment has been minimized.

@gibix

gibix Jun 13, 2018

Contributor

Sorry, but I still not understand why 1.17.0 is there. I moved in allowed failures because there will never be chance for 1.17 to build.

@@ -1,5 +1,5 @@
/*
* Copyright 2016-2017 Doug Goldstein <cardoe@cardoe.com>
* Copyright 2016-2018 Doug Goldstein <cardoe@cardoe.com>

This comment has been minimized.

@cardoe

cardoe Jun 13, 2018

Owner

I'm not making any changes in this commits. You can drop this change. You can add yourself in the commit where you make material changes to the code.

This comment has been minimized.

@gibix

gibix Jun 13, 2018

Contributor

Ok, I thoght was correct for you as the project-owner to have an updated copyright

[badges]
travis-ci = { repository = "cardoe/cargo-ebuild" }
[badges.travis-ci]
repository = "cardoe/cargo-ebuild"

This comment has been minimized.

@cardoe

cardoe Jun 13, 2018

Owner

What's the improvement here? I see the first form in most projects.

This comment has been minimized.

@gibix

gibix Jun 13, 2018

Contributor

personal porn-idiomatic

src/main.rs Outdated
@@ -81,89 +29,9 @@ Options:
fn main() {
let config = Config::default().unwrap();
let args = env::args().collect::<Vec<_>>();
let result = cargo::call_main_without_stdin(real_main, &config, USAGE, &args, false);
let result = cargo::call_main_without_stdin(cargo_ebuild::real_main, &config, USAGE, &args, false);

This comment has been minimized.

@cardoe

cardoe Jun 13, 2018

Owner

This is just wrapping a main() function into the library. Let's work out a real API and have lib.rs expose that API and then switch the binary to using that. A better way to structure the changes would be commits that work in the following order:

  • add new API
  • implement & test
  • use new API
Cargo.toml Outdated
@@ -17,7 +17,7 @@ Generates an ebuild for a package using the in-tree eclasses.
repository = "cardoe/cargo-ebuild"
[dependencies]
cargo = "^0.21"
cargo = "^0.27"

This comment has been minimized.

@cardoe

cardoe Jun 13, 2018

Owner

What's the gain from newer cargo?

src/main.rs Outdated
extern crate cargo_ebuild;
extern crate cargo;
#[macro_use]
extern crate human_panic;

This comment has been minimized.

@cardoe

cardoe Jun 13, 2018

Owner

So if someone inputted the wrong arguments we're not going to show them the following:

Well, this is embarrassing.

cargo-ebuild had a problem and crashed. To help us diagnose the problem you can send us a crash report.

We have generated a report file at "/var/folders/zw/bpfvmq390lv2c6gn_6byyv0w0000gn/T/report-8351cad6-d2b5-4fe8-accd-1fcbf4538792.toml". Submit an issue or email with the subject of "human-panic Crash Report" and include the report as an attachment.

- Homepage: https://github.com/cardoe/cargo-ebuild
- Authors: Doug Goldstein <cardoe@cardoe.com>

We take privacy seriously, and do not perform any automated error collection. In order to improve the software, we rely on people to submit reports.

Thank you kindly!

This is the wrong crate for the job. Printing out a reasonable error message worked previously and this isn't reasonable.

_ => LogLevel::Trace,
}.to_level_filter(),
)
.try_init()?;

This comment has been minimized.

@cardoe

cardoe Jun 13, 2018

Owner

We have nothing using log macros. What value is this providing?

This comment has been minimized.

@gibix

gibix Jun 13, 2018

Contributor

all the warns in cargo_ebuild::build are log. Log will be used more in future.

@gibix

This comment has been minimized.

Contributor

gibix commented Jun 13, 2018

I added the cargo bump commit because has not already been decided to drop cargo for cargo_metadata. I tried to have all the commit atomic in order to be reversable.

@cardoe

This comment has been minimized.

Owner

cardoe commented Jun 30, 2018

As we discussed off this ticket, I wanted to provide you with some commits that try to show some detailed commit messages along with code changes that should compile and pass tests at each commit. So I picked some tasks that I felt were mostly mechanical in nature and implemented them. Ultimately I've broken the main entry point away from the rest of code so that its testable. Lastly I've reduced the usage of the Cargo crate from the main binary to allow you to convert the library portion to cargo-metadata. Unfortunately this will result in a bit of conflicts for you but hopefully gives you an idea of the type changes I'm looking forward to in this project.

@gibix gibix closed this Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment