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

Bump Rust toolchain to 1.57, fix lints #1642

Merged
merged 6 commits into from
Dec 5, 2021

Conversation

mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented Dec 3, 2021

Description

New rust release, new lints to fix.
Some of them have uncovered little problems in the code, which are fixed at the root cause.


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent). -> will do in ibc-rs release 0.10.0 #1648

The clippy lint has a point: the enum may be too large to be efficiently
sent over a channel.
There's been a layering violation, the config module should use its own
smaller domain error. This gets rid of a clippy lint, too.
They don't seem to be used for anything, and this triggers a clippy
lint.
@mzabaluev mzabaluev added the O: code-hygiene Objective: cause to improve code hygiene label Dec 3, 2021
Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Thanks Mikhail!

Glad to finally see the clippy fixes!

pub fn new(cfg: GlobalConfig) -> Result<Self, FrameworkError> {
let filter = build_tracing_filter(cfg.log_level.to_string())?;

// Construct a tracing subscriber with the supplied filter and enable reloading.
let builder = FmtSubscriber::builder()
.with_target(false)
.with_env_filter(filter)
.with_writer(std::io::stderr as StdWriter)
.with_writer(std::io::stderr)
Copy link
Member

Choose a reason for hiding this comment

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

Just a note: This code should be tested at a basic level in the E2E python scripts, which rely on the JSON output. It would still be useful to run some CLI commands with and without --json to make sure we're not changing the structure of the output here.

We can do that, however, in the release PR (which will close #1648): on that occasion, we also would like to update the guide, which requires re-running the CLI anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a stylistic change, the cast did nothing.

Copy link
Member

Choose a reason for hiding this comment

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

It was required on a previous version of rustc, glad to see it's not needed anymore :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah hm, this means we've implicitly bumped our MSRV.
If we care about such things, it may make sense to add a clippy.toml and maintain the MSRV there.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah let's do that!

@adizere adizere merged commit 859f9dc into master Dec 5, 2021
@adizere adizere deleted the mikhail/bump-toolchain-to-1.57-fix-lints branch December 5, 2021 12:03
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Bump Rust toolchain to 1.57

* Fix trivial lints introduced in clippy 0.1.57

* Box the value in SupervisorCmd::UpdateConfig

The clippy lint has a point: the enum may be too large to be efficiently
sent over a channel.

* Factor config errors out into a smaller error type

There's been a layering violation, the config module should use its own
smaller domain error. This gets rid of a clippy lint, too.

* Remove reload handles for tracing subscribers

They don't seem to be used for anything, and this triggers a clippy
lint.

* relayer-cli: Remove trivial casts in components
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O: code-hygiene Objective: cause to improve code hygiene
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants