Skip to content

[RFC] Rename send to write. #281

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

Merged
merged 1 commit into from
Jun 15, 2021
Merged

[RFC] Rename send to write. #281

merged 1 commit into from
Jun 15, 2021

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented May 28, 2021

send and write are essentially synonymous. All the traits use write, except nb::spi::FullDuplex, which uses send.

This PR renames it to write. Instances of send in the docs are also replaced, for consistency.

@Dirbaio Dirbaio requested a review from a team as a code owner May 28, 2021 22:05
@rust-highfive
Copy link

r? @therealprof

(rust-highfive has picked a reviewer for you, use r? to override)

Copy link
Contributor

@ryankurte ryankurte left a comment

Choose a reason for hiding this comment

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

lgtm

@eldruin
Copy link
Member

eldruin commented May 29, 2021

This would lead to ambiguous calls for users of the prelude since all traits are pulled in.
The current method names are all different for traits in blocking and non-blocking modes. See for example SPI:

  • dev.read() -> nb::spi::Read::read()
  • dev.send -> nb::spi::Write::send()
  • dev.transfer() -> blocking::spi::Transfer::transfer()
  • dev.write -> blocking::spi::Write::write()

Same thing for serial in #280.
Choosing the same name would force users to either stop using the prelude or turbofish calls.
We could also get rid of the prelude, though.

@Dirbaio
Copy link
Member Author

Dirbaio commented May 29, 2021

If you look at that "merged" API as a whole, it feels very inconsistent:

  • write is blocking, read is nonblocking.
  • there's send and write that look like they do the same.

I think having equal names for equivalent blocking/nonblocking methods is much clearer.

About the naming conflicts:

  • Hal-agnostic code will use either the blocking or nonblocking trait, but not both, so it won't run into conflicts.
  • Code using a particular HAL can avoid the conflict by importing only one trait.
  • Also, as per the discussions in Ship a 1.0 release #177 (comment) and Remove try_ prefix. #268 HALs can define inherent methods of the same names with ergonomic improvements (infallible, for example) so end-user code doesn't have to use any e-h trait at all.

Given the above, maybe removing the prelude is a good idea, yup. I don't see it adding ergonomic improvements in either hal-agnostic or hal-using code.

Also, the Rng traits already have read for both blocking and nonblocking :)

@eldruin
Copy link
Member

eldruin commented May 31, 2021

The problem only exists for structs implementing the blocking and non-blocking versions of the traits.
For example, an SPI MCU peripheral. This would implement both versions so that the user can use whatever necessary, hence ambiguity if both traits are imported.
For unrelated traits this is irrelevant because the same struct would not implement both Rng and SPI traits.

@Dirbaio Dirbaio mentioned this pull request Jun 15, 2021
@Dirbaio
Copy link
Member Author

Dirbaio commented Jun 15, 2021

This (and #280) was discussed in last week's and today's WG meetings. Minutes:

    * both PRs discussed, so far conclusion is:
            * remove prelude so we can easily have overlapping method names between traits
            * update traits to make send/write/bwrite/etc methods all consistent across blocking/nonblocking traits
            * essentially, approve both these PRs and also remove the prelude

The key question is "should different flavors of the same trait use the same method names?". Arguments in favor and against are pretty much the ones already discussed in this thread.

One argument against is the prelude, the solution is removing it (#282).

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Fort the record, the decisive argument for me was that it would be weird when we have 3 different synonyms for write, one for blocking, one for nb, one for futures. So it seems better to remove the prelude and disambiguate if necessary.

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.

LGTM

bors r+

@bors bors bot merged commit 4ff73e7 into rust-embedded:master Jun 15, 2021
bors bot added a commit that referenced this pull request Jun 29, 2021
282: Remove prelude. r=therealprof a=Dirbaio

**DEPENDS on #280**

Following discussions in #280 #281, we might want to use the same method names for the `blocking` and `nb` (and in the future, `futures`) trait flavors.

In that case, importing the prelude guarantees naming conflicts, so it's better to remove it.

An alternative would be splitting the prelude in `blocking` and `nb`. However, users often use blocking trait A and nonblocking trait B at the same time, so they'd end up importing both preludes and having name conflicts anyway.

Co-authored-by: Dario Nieuwenhuis <[email protected]>
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.

5 participants