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.

Sign up for GitHub

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

Jump to bottom

prepare v0.3.0: change API to Iterator, implement core::error::Error & add some Copy, Clone, Hash where applicable #38

Merged
merged 7 commits into from
Oct 13, 2024
Merged

prepare v0.3.0: change API to Iterator, implement core::error::Error & add some Copy, Clone, Hash where applicable #38

merged 7 commits into from
Oct 13, 2024

Conversation

rursprung
Copy link
Collaborator

@rursprung rursprung commented Sep 30, 2024 •
edited
Loading

see the individual commit messages for further details.

this is one step towards an eventual v1.0 release, the only missing thing after this will be stable dependencies (see #25)

NOTE:

  • once the PR has been ACKed i'll update the settings for the required GHA actions and do the release and then push the release commit to this PR again (will need a re-ACK to get it merged, but it'll already be on crates.io)
  • i didn't test this on hardware but this isn't necessary as the crate is hardware-independent and has test coverage (+ i don't have the BT module anymore to test with)

@rursprung
simplify GHA required build settings
09c49dd 
by adding a single `build-results` job which depends on all other jobs
we can simplify the setting of required builds in the repository.
currently, all builds - including all variations of the build matrix! -
need to be manually specified.

once this has been merged the settings can be changed to require only
this one job (which will fail if any of the other jobs failed).

this way it's also easier to add/remove jobs or change the build matrix
as it no longer requires changing the settings on the repository.

this is inspired by [this discussion on GH][discussion].

[discussion]: https://github.com/orgs/community/discussions/26822
@rursprung
implement core::error::Error
abe2f20 
this API has been stabilised with Rust 1.81.0. accordingly the MSRV has
been raised.
@rursprung
rename use_.. features
4dab854 
this resolves the [C-FEATURE] finding from the API guidelines which
states:
> Do not include words in the name of a Cargo feature that convey zero
> meaning, as in `use-abc` or `with-abc`. Name the feature `abc`
> directly.

note that this is a breaking change for existing consumers which must
rename their usage of the feature.

[C-FEATURE]: https://rust-lang.github.io/api-guidelines/naming.html#c-feature
@rursprung
add Copy, Clone and Hash on error & event types
29cca90 
note that `Hash` cannot be added to types which use floats underneath.
for the same reason `Eq` cannot be added (but `PartialEq` is present).

this resolves [C-COMMON-TRAITS] of the API guidelines.

[C-COMMON-TRAITS]: https://rust-lang.github.io/api-guidelines/interoperability.html#c-common-traits
@rursprung rursprung requested a review from eldruin September 30, 2024 18:00
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@rursprung
Copy link
Collaborator Author

as an alternative to doing a v0.3.0 now and v1.0.0 at an unknown future date i could also add a statement to the README labelling optional 0.x dependencies (all 0.x dependencies are optional) as being out-of-scope of regular semver guarantees and that any breaking change there can happen in a major release here until they reach 1.0. WDYT?

@jannic
Copy link
Contributor

jannic commented Oct 12, 2024

once the PR has been ACKed i'll update the settings for the required GHA actions and do the release and then push the release commit to this PR again (will need a re-ACK to get it merged, but it'll already be on crates.io)

I don't understand why you won't merge this PR, do the release, and create a new PR for the release commit?

jannic
jannic reviewed Oct 12, 2024
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
@rursprung
Copy link
Collaborator Author

I don't understand why you won't merge this PR, do the release, and create a new PR for the release commit?

either works fine, there's no difference. i can go with the new PR, that's fine 👍

the more i'm thinking about this the more i think that the parser should maybe be implemented as an iterator. in that case i also wouldn't need the dependency to heapless. WDYT?

so i went ahead and prototyped this:
e6717c5

i think that's a much more idiomatic rust API. i'm not sure why i hadn't had this idea before!
if you agree that this would be a better way i'll push that commit onto this PR

@rursprung rursprung changed the title prepare v0.3.0: implement core::error::Error, rename use_.. features & add some Copy, Clone, Hash where applicable prepare v0.3.0: change API to Iterator, implement core::error::Error & add some Copy, Clone, Hash where applicable Oct 13, 2024
@rursprung
Copy link
Collaborator Author

i've now completely changed the API to use an Iterator, thus removing the need to return a Vec (and accordingly the need for either alloc or heapless 🥳)

so the main question remaining now will be: should this be v0.3.0 or v1.0.0 (with later major releases when the optional dependencies rgb and defmt become stable)?

jannic
jannic reviewed Oct 13, 2024
View reviewed changes
.github/workflows/CI.yml Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@rursprung
rework API to use implement Iterator
6240970 
this completely revamps the API. the `parse` function is gone, instead
you now construct a new `Parser` using `Parser::new` and then use the
iterator API on it which will yield a `next` until the whole input has
been parsed.

due to this there's now no longer a need to return a list of values from
the API as instead we return the individual events (or errors) step by
step. thus the dependency on `alloc` and `heapless` has been removed.

to get the same result as before (everything collected into a vector)
the `collect` API of `Iterator` can be used by consumers.
@rursprung rursprung merged commit 53187a6 into rust-embedded-community:master Oct 13, 2024
5 checks passed
@rursprung rursprung deleted the prepare-v0.3.0 branch October 13, 2024 15:40
@rursprung rursprung mentioned this pull request Oct 13, 2024
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jannic jannic jannic approved these changes

@eldruin eldruin Awaiting requested review from eldruin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rursprung @jannic