Skip to content

Reduce technical debt #153

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

Merged
merged 22 commits into from
May 18, 2022
Merged

Reduce technical debt #153

merged 22 commits into from
May 18, 2022

Conversation

kraktus
Copy link
Contributor

@kraktus kraktus commented May 13, 2022

No functional change

Replace deprecated/no-longer-maintained crates with a up-to-date replacement:

  • structopt -> clap allowing for more validation to be done during arguments passing
  • failure -> thiserror + anyhow

along with more tests and some refactoring.

this PR has maybe grown too big, and I can split it if needed, although efforts were made for it to be reviewed on a per-commit basis.

kraktus added 19 commits May 4, 2022 19:08
Important before moving from `StructOpt` to `clap`
`failure` is deprecated in favour of these two librairies

Update `Cargo.lock` and `Cargo.toml`
by anyhow::Result<_> or true error type when possible
`Structopt` is now in maintenance-only mode and advise to use `clap` instead
Made possible with using `thiserror`
This will also allow to display the correct default value with `--help`

`--host` and `--target` have the same goal and probably `--host` should be removed but for a later work
@kraktus
Copy link
Contributor Author

kraktus commented May 13, 2022

CLI snapshots tests are failing on windows, I'll investigate but I think it's an issue with the test rather than with the behaviour

@oli-obk
Copy link
Contributor

oli-obk commented May 16, 2022

General note: I think I would prefer eyre (with color-eyre) over anyhow, but that's not a strong requirement

Not to pass `Config` and `Client` every time
@kraktus
Copy link
Contributor Author

kraktus commented May 18, 2022

CI fixed, and previous comments should have been addressed.

General note: I think I would prefer eyre (with color-eyre) over anyhow, but that's not a strong requirement

I am not familiar with the library, from what I see the difference is that it includes spantrace. It will be a bit more noisy but considering our target are developers it should not be an issue. If we're good with leaving it by default, can make the switch.

@oli-obk oli-obk merged commit 51b0484 into rust-lang:master May 18, 2022
@oli-obk
Copy link
Contributor

oli-obk commented May 18, 2022

Thanks!

I went ahead and merged it, since it can stand on its own.

Yea, using spantrace and being more developer focussed is good here, as failures in the tool are likely going to be debugged by users of the tool as you noted. Let's do that in a separate PR tho

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