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

Re-design the CLI interface #239

Merged
merged 9 commits into from
Sep 21, 2022

Conversation

jessebraham
Copy link
Member

@jessebraham jessebraham commented Sep 16, 2022

This PR includes a re-design of the command-line interface, most notably adding a flash subcommand.

In the current interface, since there is no flashing subcommand, it's possible to pass arguments before a subcommand which are accepted but silently ignored. This is bad UX and is a common source of confusion for users. Adding a flashing subcommand eliminates this issue entirely.

While I was breaking things, I did a bit of refactoring as well. This was mostly just moving types to the module they belong in, simplifying some functions, etc. Nothing too interesting. I've also renamed a number of types/fields/functions/etc. to be more in line with other parts of the code, eg.) CLI arguments now use arg instead of opt, as their types are even clap::Arg.

Additionally I have created a cli feature which all CLI-related dependencies are gated behind. This allows these packages to be excluded when using espflash as a library. This feature is enabled by default. The executable was renamed from main.rs to bin/espflash.rs to accommodate this.

Beyond addressing whatever changes are requested, I'm not sure that there is much more to add to this PR. There are additional UX-related changes I would like to make but they are out of scope and deserve their own PRs, I think.

I would like to spend some time reviewing this and really picking it apart before we merge it. I have marked this as a draft until I am comfortable with its state. Small details such as short/long names, docstrings (which are displayed in help messages) and argument names are important, so please nitpick!

Closes #222
Closes #236

@hnidoaht-101
Copy link

hi @jessebraham I come here via esp-rs chat channel. Its clear that I'm totally newbie, so please forgive if I have some naive questions 😅. Is there any reasons you decided to switch from tracing-subcriber crate to env-logger?

@jessebraham
Copy link
Member Author

@hnidoaht-101 for context, tracing-subscriber was added in a recent PR, we did not have any logging prior and I just really didn't scrutinize this inclusion (partially because I was already working on revamping the CLI anyways, so it didn't really matter much).

env-logger is what I would normally use in these situations, and it's a much simpler package. I frankly had never even heard of tracing-subscriber before that PR, so if there is some tangible benefit in using it then I would consider reverting, but I'm not sure what said benefit(s) would be.

@bjoernQ
Copy link
Contributor

bjoernQ commented Sep 19, 2022

Haven't looked much into this yet but definitely thumbs-up for adding a flash command and the other changes described here

espflash/src/cli/mod.rs Outdated Show resolved Hide resolved
@jessebraham jessebraham marked this pull request as ready for review September 21, 2022 15:08
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - also I'm using it for a few days without issues

Maybe we should bump the version in this PR already? No needed but I think it would be nice

@jessebraham
Copy link
Member Author

Thanks for taking a look!

I wasn't sure when to bump the version, I can set it to 2.0.0-dev for now though I suppose. I do plan on submitting a handful more PRs prior to releasing (mostly just refactoring, but now is the time to break public APIs if we're gonna do it!)

@jessebraham jessebraham merged commit 812749e into esp-rs:master Sep 21, 2022
@jessebraham jessebraham deleted the feature/cli-rewrite branch September 26, 2022 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants