Skip to content

chore: multiple minor cleanups #872

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

nyurik
Copy link

@nyurik nyurik commented May 10, 2025

Per @ianrrees review (thx!), this consolidates #860 #861#862

consolidate workspace settings

  • Ensure all crates are consistently configured and validated using workspace features
  • move edition, license, repo, and rust-version (where used) to workspace
  • remove readme setting (already set by default)
  • add lints section - this should be used in a separate PR

Code changes

  • Migrate several examples to 2021 edition

Other changes

  • crates.json reformatted for consistency and ease-of-reading
  • Reformat all Cargo.toml that have already been modified with consistent spacing using intellij built-in formatter (pretty similar to what was already used, and it is being proposed for the toml formatting in rustfmt, but that has stalled)
  • Minor docs cleanup for syntax, style, and address @ianrrees text suggestions

### consolidate workspace settings
* Ensure all crates are consistently configured and validated using workspace features
* move edition, license, repo, and rust-version (where used) to workspace
* remove readme setting (already set by default)
* add lints section - this should be used in a separate PR

### Code changes
* Migrate several examples to 2021 edition

### Other changes
* `crates.json` reformatted for consistency and ease-of-reading
* Reformat all Cargo.toml that have already been modified with consistent spacing using intellij built-in formatter (pretty similar to what was already used, and it is being proposed for the toml formatting in rustfmt, but that has stalled)
Copy link
Contributor

@ianrrees ianrrees left a comment

Choose a reason for hiding this comment

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

Thanks for the contributions @nyurik and apologies for my taking a few days to follow up. I'd hoped that by combining the three earlier PRs that we'd get a clearer view of changes you're requesting, and by keeping the commits separate we'd be able to easily pick and choose between them.

I appreciate the docstring cleanups, but to be frank I think most of the rest of this is a bit questionable value (eg whitespace changes) or problematic (eg tying tier 2 BSP config to the workspace). Probably best to discuss details in chat.

repository = "https://github.com/atsamd-rs/atsamd"
rust-version = "1.77.2"

[workspace.lints.rust]
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for adding these empty lints sections?

repository = "https://github.com/atsamd-rs/atsamd"
readme = "README.md"
edition = "2021"
edition.workspace = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a "tier 2" BSP, I don't think we want to use the wider workspace settings for it, that kinda defeats the purpose of the distinction from tier 1.

version = "0.19.0"
edition.workspace = true
license.workspace = true
repository.workspace = true
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if the repository link for BSPs shouldn't actually go to eg https://github.com/atsamd-rs/atsamd/tree/master/boards/metro_m0

"defmt",
"async"
],
"features": [ "samd11c", "dma", "defmt", "async" ],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to change the formatting in this file?

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