Skip to content

Conversation

nyurik
Copy link

@nyurik nyurik commented May 6, 2025

Summary

Just a few cleanups - trying to make things a bit more readable

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.

I appreciate your recent interest and PRs, @nyurik ! The currently open three PRs are for relatively small changes, it would help if you could put them in to one PR - maybe using different commits to separate the different type of change.

//! ## Features
//!
//! - Converting between pin modes no longer requires access to the `Port` type.
//! - Converting between pin modes no longer requires access to the
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just get rid of this whole reference to the old GPIO module - it was useful when we had two GPIO versions in the HAL but now only of historical interest

//! A: EnumOne,
//! B: EnumTwo,
//! {
//! struct Container<A: EnumOne, B: EnumTwo> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are this and the other changes to eliminate the where clauses personal preference, or suggested by a tool? I feel the old way is more illustrative, and since they are in docstrings this change seems to be moving in the wrong direction.

@nyurik
Copy link
Author

nyurik commented May 7, 2025

@ianrrees , thx for such a quick review! The where clause was not suggested by any tool, but rather me reading through the docs and stumbling on this, which caused me to create this PR (which included a few minor doc changes). It is totally a personal preference that if the code can be expressed concisely, it should be, until it no longer fits on a single line - and only then it should be broken up into the where clause. I was reading all the docs carefully, and it took me a moment to get through the syntax - hence the reason for the PR - just trying to make it a bit easier to read.

I am happy to make a bigger single PR - some projects simply require smaller and easier to review changes - e.g. if i am editing the docs, i shouldn't change the cargo.toml - as that may hide a bug - but i will be happy to merge all PRs into one

@nyurik
Copy link
Author

nyurik commented May 10, 2025

closing in favor of #872

@nyurik nyurik closed this May 10, 2025
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