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

[towards 1.0]: Fallible proven traits #192

Merged
merged 22 commits into from
Mar 16, 2020

Conversation

eldruin
Copy link
Member

@eldruin eldruin commented Mar 11, 2020

Here some work towards 1.0 following #177 (comment).

@rust-highfive
Copy link

r? @thejpster

(rust_highfive has picked a reviewer for you, use r? to override)

@Disasm
Copy link
Member

Disasm commented Mar 11, 2020

We agreed on try_ prefix for fallible methods to prevent name collision.

@eldruin
Copy link
Member Author

eldruin commented Mar 12, 2020

Oops, I misread that. Here some more work towards what was discussed and thus 1.0.

  • Rename all methods try_* and make them fallible as appropriate.
  • Use core instead of std.
  • Remove unproven feature

I did not rename the adc::Channel::channel() method. This method exists at all due to rust-lang/rust#54973, which is fixed as of Rust 1.35.
What do we want to do about this?

  • Keep as is.
  • Rename method.
  • Update MSRV to 1.35 and make the method a constant.

@eldruin eldruin changed the title Fallible traits [towards 1.0]: Fallible proven traits Mar 12, 2020
@thejpster
Copy link
Contributor

Personally I'm OK with upping the MSRV to 1.35. That is now 7 versions (or 42 weeks) behind the current stable.

src/blocking/delay.rs Show resolved Hide resolved
src/timer.rs Show resolved Hide resolved
@Disasm
Copy link
Member

Disasm commented Mar 14, 2020

I think that adding try_ prefix only applies to traits that are already fallible. I'm not sure if we should convert all infallible traits into fallible ones as this can be done later by adding another fallible trait.

@eldruin
Copy link
Member Author

eldruin commented Mar 14, 2020

I think that adding try_ prefix only applies to traits that are already fallible. I'm not sure if we should convert all infallible traits into fallible ones as this can be done later by adding another fallible trait.

Hmm, this would mean we would be back to PwmPin vs. FalliblePwmPin, which was extensively discussed and discarded for the OutputPin case (#95, #97, #100, #105).

Here a relevant excerpt from the meeting protocol:

  1. For the next major release, we will ONLY be offering Fallible traits, e.g. traits that return Results. All trait method names will be prefixed with try_ for consistency, e.g. try_send.

@thejpster
Copy link
Contributor

LGTM. Any reason we can't merge this?

[dependencies.nb]
version = "0.1.1"
[dependencies]
nb = { version = "0.1.1", features = ["unstable"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

unstable ? That has a weird sound to it considering that we're aiming to stabilize this.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is because currently the unproven feature requires nb/unstable. The state of the unstable feature should be discussed for nb's 1.0 release.

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

Other than the 1 nit I mentioned inline looks good to me.

@thejpster
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 16, 2020

Build succeeded

@bors bors bot merged commit 88412b9 into rust-embedded:master Mar 16, 2020
@eldruin eldruin deleted the fallible-traits branch March 16, 2020 06:51
@ryankurte
Copy link
Contributor

Sorry to raise this a bit late, but would it make sense for us to batch all the breaking change PRs into a separate branch so that we can release bug fixes (for example #194) prior to the next major release? Adds to workload in keeping the branch up to date but avoids this stalling us if this takes a little while.

@thejpster
Copy link
Contributor

I'm a big fan of git flow for release management, but that didn't go down well in a recent meeting. Shall we put something about rules for branch management on the agenda for the next meeting?

@thejpster
Copy link
Contributor

For now if you need to make a hotfix, I suggest you create a branch from the release tag.

@therealprof
Copy link
Contributor

Sorry to raise this a bit late, but would it make sense for us to batch all the breaking change PRs into a separate branch so that we can release bug fixes (for example #194) prior to the next major release? Adds to workload in keeping the branch up to date but avoids this stalling us if this takes a little while.

I agree we should try to keep the individual PRs as compact and self-contained as possible to allow for cherry-picking if necessary for e.g. a bugfix release.

I'm a big fan of git flow for release management, but that didn't go down well in a recent meeting.

Well, it would be sufficient if the reviewer insisted on broken down PRs before accepting a wholesale crate PR. 😅However in this particular case we're good because the author took good care and split the change into plenty of individual commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants