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

Add error propagation from the embedded_hal functions #37

Merged

Conversation

ripytide
Copy link
Collaborator

@ripytide ripytide commented Jan 15, 2024

Also I though it might be nice to reduce the API surface by removing the drive_forward, drive_backward, stop and brake functions which make the library harder to maintain and also adds confusing redundancy as it leaves two ways of accomplishing the same thing, drive() vs the other commands.

I also renamed DriveCommand::Backwards to DriveCommand::Backward to match the non-plural DriveCommand::Forward command.

Copy link
Collaborator

@rursprung rursprung left a comment

Choose a reason for hiding this comment

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

thank you for your contribution!

some requests from my side (besides the comments in the code):

  • please split this into atomic commits (and possibly atomic PRs; i'm still a bit on the fence about the last point)
    • one for the error propagation
    • one for Backwards => Backward (thanks for catching that!)
    • one for removing the APIs
  • please use proper commit messages (also mentioned in the Git Book)

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@ripytide
Copy link
Collaborator Author

While in future I will try to note your preference for atomic commits (apologies I forgot since last time you asked), I think it would be not worth the effort for me to split this into multiple commits as I'd have to re-do everything from scratch again.

I don't think its generally a great idea to expect open-source contributors (particularly new contributors) to stick to such a commit-schema as it may discourage committers without enough know-how of git from making pull requests.

src/lib.rs Outdated Show resolved Hide resolved
@rursprung
Copy link
Collaborator

While in future I will try to note your preference for atomic commits (apologies I forgot since last time you asked), I think it would be not worth the effort for me to split this into multiple commits as I'd have to re-do everything from scratch again.

I don't think its generally a great idea to expect open-source contributors (particularly new contributors) to stick to such a commit-schema as it may discourage committers without enough know-how of git from making pull requests.

i think it's important to (try to) uphold the standards also (and especially) in open source projects, both to make them maintainable & understandable in the long-term but also to teach people the best practices.

for this specific PR i think it's fine if you squash the commits together and provide a nice commit message (you can go with a generic "refactor: streamline API and expose errors through Result" and then add a nice description to it - this way you can always claim that it's atomic enough 😉)

i think i'm ok with removing the duplicate methods. for the Result thing i'll briefly ping the rust embedded matrix room to see what the general consensus/recommendation on this is (seeing that the Result thing is a very new thing on the pins).

@rursprung
Copy link
Collaborator

@ripytide: sorry for the long delay here! rust-embedded/embedded-hal#576 has been discussed in the last rust-embedded meeting (see the meeting minutes) and the approach chosen here is considered best practices for no_std (with std there are cleaner/easier options).

could you please:

  • rebase the PR
  • squash everything into one commit with a nice commit message (see previous comment)
    ?

with that i can then ACK & merge it and we can get your other PR in as well and finally release this thing!

Also:
- renamed error types to their struct names
- renamed DriveCommand::Backwards to DriveCommand::Backward to match
  DriveCommand::Forward
- removed the `drive_forward`, `drive_backward`, `stop` and `brake`
  functions as they are duplicates to the `drive` function with the
  different enum variants and make the API surface larger
- cleaned up the changelog
@ripytide
Copy link
Collaborator Author

ripytide commented Mar 3, 2024

@rursprung I have rebased and squashed the PR

@ripytide ripytide linked an issue Mar 3, 2024 that may be closed by this pull request
Copy link
Collaborator

@rursprung rursprung left a comment

Choose a reason for hiding this comment

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

thanks a lot for this!

@rursprung rursprung merged commit bd213c8 into rust-embedded-community:master Mar 6, 2024
7 checks passed
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.

embedded_hal errors shouldn't be ignored
2 participants