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 AsByte trait and satisfy, one_of and none_of for bytes #1789

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SneedSeedFeed
Copy link

I just wish to preface: This is my first attempt at an open source contribution so apologies if I get things wrong, please assume ignorance or stupidity for any mistakes instead of malice.

The problem

I was writing a parser and wanted satisfy for bytes, but noticed there was only a character version that I ultimately could've reused but it just rubbed me the wrong way doing so.

My solution

  • Adding an AsByte trait for &u8 and u8
  • Adding a near 1:1 copy of satisfy, one_of and none_of for bytes::{streaming, complete}.
  • Amending the test dependent on character::complete::{one_of, none_of} to specify the exact function and avoid ambiguity

Issues with my solution

  • AsByte doesn't really do much, adding a trait with a single function and only two implementers didn't feel right but I felt parity with the character version was more important to meet the standards of existing code.
  • The name AsByte overlaps a bit with AsBytes. To avoid changing the name of AsBytes to something like AsByteSlice, could maybe change AsByte to IsByte especially since the only two implementers are both u8 it would be arguably more descriptive.
  • Vaguely the same effect could be achieved by adding pub use characters::_::{satisfy, one_of, none_of} to the respective module in bytes, with the benefit of providing slightly less code to maintain.
  • As I wanted to copy the character implementation, the tests in the doc comments are a little ugly to cast the &[u8; N] to &[u8]
  • I copied the character docs and wrote over it so I may have left some typos or uses of char over u8 in there.

… rename one)

Added satisfy, one_of and none_of to bytes::complete
Added satisfy, one_of and none_of to bytes::streaming
Ammended test for issue rust-bakery#1118 to specify the use of character::complete::{none_of, one_of} due to additions to bytes::complete
@SneedSeedFeed SneedSeedFeed requested a review from Geal as a code owner November 28, 2024 19:30
@SneedSeedFeed SneedSeedFeed changed the title Added AsByte trait (name overlaps a little with AsBytes, maybe should… Add AsByte trait and satisfy, one_of and none_of for bytes Nov 28, 2024
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.

1 participant