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

Refactor: Parse traits #11

Merged
merged 6 commits into from
Dec 28, 2023
Merged

Refactor: Parse traits #11

merged 6 commits into from
Dec 28, 2023

Conversation

steffahn
Copy link
Contributor

@steffahn steffahn commented Dec 28, 2023

Hi there, I’ve been noticing the weird-looking trait bounds appearing on the public API, such as pub fn parse<T: Parser<T> + ParserPos<T>>.

The trait’s type parameters aren’t actually necessary. Perhaps they were used because the author didn’t realize they could achieve the default implementations for Parse, which used to rely on a trait bound in the argument, trait Parser<T: ParserPos<T>>, by using a supertrait bound instead.

Anyways, this PR contains multiple commits, starting off with

  • removal of the redundant generic argument on Parser, ParserPos, ParserNeg
    • so it’d be pub fn parse<T: Parser + ParserPos> then
  • renaming of these traits to use “parse” instead of “parser” – this will look a lot more fitting in the documentation, in my opinion
    • so it’d be pub fn parse<T: Parse + ParsePos> then
  • simplification of the bounds, another thing that becomes possibly with the supertrait bound trait Parse: ParsePos
    • so it’d be pub fn parse<T: Parse> then
  • using only Parse and ParseNeg, no longer ParsePos, in public API. The set of types implementing Parse and ParsePos seems to be identical. (Please comment if this observation is wrong under any cfgs, or is intended to become wrong in the future.)
    • this turns e.g. pub fn parse_pos<T: ParsePos> into pub fn parse_pos<T: Parse>
  • I could not help but stumble over the impl_signed macro, which I felt was more tidy if the impl was created by the macro fully, which also stylistically follows common precedent in std – sorry this is relatively unrelated to the main refactor here, but it’s still something about the Parse[r] trait I’ve come across

Feel free to give feedback in case you’re interested in merging only part of these changes.

@steffahn steffahn changed the title Minor cleanup around parser traits Some cleanup/refactor of Parser… trait bounds Dec 28, 2023
@RoDmitry RoDmitry merged commit eef9970 into RoDmitry:master Dec 28, 2023
5 checks passed
@RoDmitry
Copy link
Owner

RoDmitry commented Dec 28, 2023

Thank you! I like it 😊

@steffahn steffahn deleted the minor_cleanup_around_parser_traits branch December 28, 2023 12:06
@RoDmitry RoDmitry changed the title Some cleanup/refactor of Parser… trait bounds Refactor: Parse traits Dec 28, 2023
@RoDmitry
Copy link
Owner

It's not a public API though 😁

@steffahn
Copy link
Contributor Author

steffahn commented Dec 28, 2023

My bad, my wording wasn’t the best perhaps, I meant that they appear visibly in the trait bounds in the public API’s documentation as created by rustdoc. I’m aware that the traits themselves are not part of the public API.

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.

None yet

2 participants