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

Upgrade to Winnow 0.7 #1822

Merged
merged 4 commits into from
Feb 1, 2025
Merged

Upgrade to Winnow 0.7 #1822

merged 4 commits into from
Feb 1, 2025

Conversation

epage
Copy link
Contributor

@epage epage commented Jan 31, 2025

FYI as this doesn't use cut_err or Partial, you could switch away from ErrMode for some potential simplification and performance improvements.

I think it should just be

  1. Remove ErrMode
  2. Switch from ModalParser to Parser
  3. Switch from ModalResult to winnow::Result

However, it'd be harder to undo it (picking apart which Results need to be ModalResults) so I held off on it from this PR

Copy link
Member

@Byron Byron 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 your tremendous help with winnow updates, it's much appreciated!

It seems using winnow::Result is lighter as it doesn't include ErrMode, so it might be good for performance improvements. For now I don't know what that would do to the error output, but it seems that can be maintained as is.

Additionally I have left a comment with a question on how to improve a particular piece of code, hoping you find the time to take a look.

Thanks again.

&start,
StrContext::Label("Closing '>' not found"),
)))?;
let right_delim_idx = i[..eol_idx]
Copy link
Member

Choose a reason for hiding this comment

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

This change was introduced in #1439 and I wonder if this there isn't an 'packagiomatic' way of writing this. This code implements the parser by hand, but that was done merely due to lack of knowledge on how to use winnow primitives.

Is there anything that could be done to make it nicer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main challenge is the rfind, right? I've not thought of a good way to handle cases like that.

If its parsing within the \n delimited range, there is and_then, like take_until(0.., b'\n').and_then(parser).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's the rfind() to assure it finds the right-most > as delimiter, within the line boundary.

@Byron Byron merged commit 11ac79c into GitoxideLabs:main Feb 1, 2025
21 checks passed
@tisonkun
Copy link
Contributor

tisonkun commented Feb 1, 2025

I happened to look into this patch and would like to share some thoughts on ModalResult vs Result.

  1. Currently, the gix_actor::identity::decode::identity parser returns ErrMode inline, so ModalResult and ModalParser are still needed.
  2. However, the identity parser doesn't used in alt/opt, so perhaps directly returning an error without the ErrMode wrapper is possible. (won't hurt error reporting, in theory)
  3. But this is like a one-way door as mentioned in the description ("harder to undo it").

Back to the reason I jump into this patch: this is the final winnow 0.6 dependency in the tree of my application, so I'd expect a new release to eliminate that :D

@epage epage deleted the w7 branch February 3, 2025 21:08
@epage
Copy link
Contributor Author

epage commented Feb 3, 2025

But this is like a one-way door as mentioned in the description ("harder to undo it").

I do see it less likely that gitoxide will need ErrMode. Gitoxide is mostly focused on binary format parsing. cut_err is mostly important when wanting to improve the quality of error messages which is usually for parsing text.

@epage
Copy link
Contributor Author

epage commented Feb 3, 2025

Oh, #1439 added Cut, nm.

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.

3 participants