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

test the Streaming wrapper on HTTP benchmarks #1614

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Geal
Copy link
Collaborator

@Geal Geal commented Jan 7, 2023

This is an experiment in merging streaming and complete implementations of parsers, using a condition on the input type, loosely inspired from winnow-rs/winnow#28

@Geal
Copy link
Collaborator Author

Geal commented Jan 7, 2023

@epage it looks like the const bool is not necessary, a static method on the Input trait is enough information for the compiler to only generate the version requested by the input type

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3863929320

  • 0 of 87 (0.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.1%) to 60.364%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/bytes/mod.rs 0 16 0.0%
src/character/mod.rs 0 23 0.0%
src/traits.rs 0 48 0.0%
Totals Coverage Status
Change from base Build 3863115493: -2.1%
Covered Lines: 1558
Relevant Lines: 2581

💛 - Coveralls

@epage
Copy link
Contributor

epage commented Jan 8, 2023

@epage it looks like the const bool is not necessary, a static method on the Input trait is enough information for the compiler to only generate the version requested by the input type

I had originally intended to just have a function on the trait but switched to const bool through development.

  • A paranoid part of me wanted the contract very clear and without ways to circumvent it
  • The const bool allows something to only be implemented for one or the other
    • Example: Distinct InputTakeAtPosition traits. I had backed off from this, mostly for an easier migration path for users. The trait merger also complicates ever doing this again.
    • Example: limiting Finish to only complete parsers. Finish panics on Incomplete and a user has to be careful with tests to realize it would do that and likely a user using a Streaming parser should be deconstructing the error anyways, so limiting Finish helps guide people to the right solution earlier in the process.

To be clear, these examples are meant to be indicative of what can be done.

Of course, going the const bool route comes at the cost of an extra generic parameter when users have to explicitly list then.

In general, I would recommend looking over my notes in #1535 as they will provide more context and might give other ideas.

@Geal
Copy link
Collaborator Author

Geal commented Jan 8, 2023

  • A paranoid part of me wanted the contract very clear and without ways to circumvent it
    Circumvent what?
  • The const bool allows something to only be implemented for one or the other
    • Example: Distinct InputTakeAtPosition traits. I had backed off from this, mostly for an easier migration path for users. The trait merger also complicates ever doing this again.
      This is not the path I'm going towards, I want to simplify here. I've tried various ways to write the low level parts of streaming and complete parsers over the years, it does not need new ways to specialize implementations. It mainly needs a way to merge them.
    • Example: limiting Finish to only complete parsers. Finish panics on Incomplete and a user has to be careful with tests to realize it would do that and likely a user using a Streaming parser should be deconstructing the error anyways, so limiting Finish helps guide people to the right solution earlier in the process.

Making it clear in the documentation that Finish can panic is enough, I don't want the implementation to overindex on a convenience trait that amounts to a match.

To be clear, these examples are meant to be indicative of what can be done.

Of course, going the const bool route comes at the cost of an extra generic parameter when users have to explicitly list then.

In general, I would recommend looking over my notes in #1535 as they will provide more context and might give other ideas.

I read them again, thank you for the context and the exploration. Here I'll choose the simpler solution though, I want that change to be minimally invasive, because I'll probably have to introduce more complex changes in that release (like the GAT idea)

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

3 participants