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(release): Switch from nom to winnow #948

Merged
merged 11 commits into from
Jul 25, 2023
Merged

Conversation

epage
Copy link
Contributor

@epage epage commented Jul 24, 2023

@Byron has mentioned interest in winnow but priorities lie elsewhere, so I'm porting the gitoxide repo over, starting with cargo-smart-release.

My expectation is that I will help with upgrades across breaking changes though I try to make that path as smooth as possible by providing both implementations of an API and deprecating the old one, rather than making hard cut offs, allowing the compiler to guide people through upgrades.

I started here because I assumed this was a smaller, less critical parser to test the waters with.

I did this the "easy path" which was to port to winnow 0.3 which has almost the exact same API as nom and then used the deprecation messages to help me through the process. I split this up into a lot of commits so that it would be easier to see what changed as rustfmt did its thing.

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 tackling this, it's much appreciated!

This is also a huge opportunity for having the parsers cleaned up by someone who knows what they are doing. After all, whenever I deal with nom parsing, I feel like I learn from scratch and resort to the common 5 things I know work, without that necessarily being the best or most efficient way, but merely a way that works.

With that said, there probably is potential performance improvements along the way and once you get to gix-object. That should probably be the last crate to handle as it's probably the one with the most parsing code, and some complexity around error handling (it has two modes, one with unit error, and one with a more detailed one, all in the name of performance).But I get ahead of myself.

One more suggestion: please refrain from using conventional git prefixes unless you these to show up in the changelogs. My policy is to only use them when it's relevant to users of the crate or application, which most certainly isn't the case here. Typically I will use a conventional commit when merging to bring one summarized message into the changelog that way.

With that said, I am looking forward to the next PRs.

@Byron Byron merged commit 37ade02 into GitoxideLabs:main Jul 25, 2023
@epage epage deleted the winnow branch July 25, 2023 13:43
@epage
Copy link
Contributor Author

epage commented Jul 25, 2023

This is also a huge opportunity for having the parsers cleaned up by someone who knows what they are doing. After all, whenever I deal with nom parsing, I feel like I learn from scratch and resort to the common 5 things I know work, without that necessarily being the best or most efficient way, but merely a way that works.

I'll keep an eye out for general factors.

Once using winnow, if you have feedback on what makes it hard to step away and come back to it, I'm all open. Would love to find ways to make that easier.

@Byron
Copy link
Member

Byron commented Jul 25, 2023

Once using winnow, if you have feedback on what makes it hard to step away and come back to it, I'm all open.

Maybe it's due to nom or winnow essentially being their own DSLs, with own ways of thinking and/or doing things. And that has to be learned, no way around it. And due to a lack of regular training, it's easy to forget the ropes.

Whenever I started a new parser, I found myself going back to the existing ones in this codebase to learn from them. So maybe, it's all about the tutorialization to learn the ropes properly, and with a good basis it will be easier to retain that knowledge and keep feeling comfortable with it.

I will let you know if something else comes to mind, the above was a quick stab at the topic.

@epage
Copy link
Contributor Author

epage commented Jul 25, 2023

Maybe it's due to nom or winnow essentially being their own DSLs, with own ways of thinking and/or doing things. And that has to be learned, no way around it. And due to a lack of regular training, it's easy to forget the ropes.

Definitely easy to forget though I think naming and organization can help.

So maybe, it's all about the tutorialization to learn the ropes properly, and with a good basis it will be easier to retain that knowledge and keep feeling comfortable with it.

I have adopted a tutorial that explains the principles and practices. Unsure if this will quite fit the role you need.

Our special topics also provide a good way to look things up by problem.

@Byron
Copy link
Member

Byron commented Jul 25, 2023

Thanks for sharing! I am sure these will come in handy the next time I have to implement one of these!

I think it's also telling that many parsers since then I implemented by hand, which probably means I have been burned by nom enough to not try again. It's this "I could do this, but don't know how to do it with X" kind of feeling I suppose.

Nonetheless, having the conversion to winnow done by you is a huge benefit as it modernizes the codebase while giving me the feeling there is an expert that can help if there is trouble.

@epage
Copy link
Contributor Author

epage commented Jul 25, 2023

I think it's also telling that many parsers since then I implemented by hand, which probably means I have been burned by nom enough to not try again. It's this "I could do this, but don't know how to do it with X" kind of feeling I suppose.

I think doing things by hand is fine.. For toml_edit, I prefer not to use too high-level of parsers because I want the code to line up directly with the BNF grammar. I generally view things like digit1 as a prototyping tool because of that.

@epage
Copy link
Contributor Author

epage commented Jul 25, 2023

Been having a couple of workflow issues

  • git status is reporting the files as changed and having to work to prevent accidentally committing the changes (lfs related)
    • This even prevented me from changing branches at one point
  • I run cargo test --all and it worked before but now its failing ingix unit tests in remote::connection::fetch::refs::tests::update (8 total)

Also, I didn't realize that gix-actor, gix-testtools, gix-object, and gix-ref are all interconnected at the nom level, so doing it as one batch. There might be subsets that I could break apart but didn't want to untangle it enough to find out

@Byron
Copy link
Member

Byron commented Jul 26, 2023

I assume that you have installed git-lfs, and that it downloaded the files it needs. On CI, that strangely enough takes an extra step. Once you have the git-lfs powered files, the modifications should go away. CI seems to be doing git lfs fetch && git lfs checkout (and I don't like that this appears to be necessary, it should be integrated, but… working on it I suppose).

About point 2, I don't know if it's related to point 1. I could probably say more if I knew the error message.

Also, I didn't realize that gix-actor, gix-testtools, gix-object, and gix-ref are all interconnected at the nom level, so doing it as one batch. There might be subsets that I could break apart but didn't want to untangle it enough to find out

I totally forgot about that and guess I took the quick and convenient route there. It's an interesting problem as well for winnow - will it encourage and provide ways of hiding such implementation details? If not, it would probably be the same situation there which probably isn't even a problem except for those moments when one wants to replace it all.

@epage
Copy link
Contributor Author

epage commented Jul 26, 2023

Yes, I ran git lfs fetch && git lfs checkout

The checkout is likely related

I totally forgot about that and guess I took the quick and convenient route there. It's an interesting problem as well for winnow - will it encourage and provide ways of hiding such implementation details? If not, it would probably be the same situation there which probably isn't even a problem except for those moments when one wants to replace it all.

Once I have a better handle on it, I'd like to look at removing winnow from 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.

2 participants