Skip to content

Conversation

@greg6775
Copy link

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo fmt command to ensure that all changed code is formatted correctly.
  • I have used cargo clippy command to ensure that all changed code passes latest Clippy nightly lints.
  • My changes were added to the CHANGELOG.md in the proper section.

Pull Request Details 📖

Description

Fixes #99

  • Updated to Rust 2024 edition
  • Updated all dependencies to latest versions
  • cargo-pio: Migrated from structopt to clap 4 derive API

Copy link
Collaborator

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

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

@greg6775 Thank you for your contribution!

There is - however - a huge amount of changed files, where the only change is re-formatting - either in the use clauses at the top, or in the file body below where some methods' arguments did become multiline.

Was this strictly necessary - if yes - why and if not why are we doing this?

I think cargo fmt is passing fine and without changes on the current code? If so, can you remove these extra formatting changes as they are really making the actual changes very difficult to spot?

Thanks!

Copy link
Collaborator

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

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

Two more things:

  • Would it be possible not to update bindgen to 0.72? bindgen is part of the embuild public API so this means 2 things I'm not sure we should do:
    • We have to release a new embuild minor version rather than a patch version
    • We have to have all esp-idf-* crates refer to the new embuild minor version

Moreover, all dependencies are updated to list explicitly a minor+patchlevel version. I.e. if something was at version "1" now it is at version "1.x.y". This is not strictly necessary unless you really want to depend on that minimum minor version and that minimum patch version in Cargo.toml. But cargo update would anyway raise those in Cargo.lock so what's the need to explicitly list those?

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.

Error creating new project

2 participants