Skip to content

Config updates #44

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

Closed
wants to merge 11 commits into from
Closed

Config updates #44

wants to merge 11 commits into from

Conversation

avitex
Copy link

@avitex avitex commented Feb 18, 2021

@Byron

I'm just starting to have a look now.

  • dangerous::Span is now used instead. This actually is less error prone because ranges could be taken from the wrong offset. dangerous::Span internally holds pointer references to solve this!
  • I've removed for now PartialOrd/Ord derives as I'm not sure what is semantic in the order, maybe you can help me here. dangerous::Span doesn't implement it because it doesn't make too much sense to me, which prevents derives. On a quick search, neither does Range Range and friends don't implement Ord/PartialOrd rust-lang/rust#75607

I'll be looking a little more though this soon I promise!

On another note, I'm thinking of making a recommendation of only using dangerous::input() for the top level and not within sub parsers. I'll be explaining my reasoning soon :)

@avitex avitex marked this pull request as draft February 18, 2021 08:00
@Byron
Copy link
Member

Byron commented Feb 18, 2021

If this is 'just looking' then I feel the thorough version of this will be a complete implementation - I wouldn't mind that at all ;).
Joking aside, thanks so much for taking a look, I am super happy with everything I see.

Especially the Span handling is so much nicer on the eyes - mine felt a bit lacking, not to say 'hacky just to make it work'. PartialOrd/Ord is nothing I am particularly interested in, but had it because I could have it. If equality would be interesting one day I am sure one could cook up an implementation by hand as well.

On another note, I'm thinking of making a recommendation of only using dangerous::input() for the top level and not within sub parsers. I'll be explaining my reasoning soon :)

My guess is that using input() multiple times allows to put in the wrong input, and thus needs some care. That's what I felt if I remember correctly.

I won't take my eyes of this PR and have it replace my branch as soon as you give the go!

Thanks again, your work is invaluable!

This was referenced Feb 18, 2021
@Byron
Copy link
Member

Byron commented Mar 13, 2021

I have to close this PR as it was superseded by a contributed implementation which is now available in main. It was originally implemented in nom but it's possible to rewrite the parser in dangerous if any roadblocks are hit.

Thanks a million for your help here ❤️, and even though I am glad to have a git-config parser available for gitoxide I would have loved to see it realized in dangerous maybe even by myself 😅.

@Byron Byron closed this Mar 13, 2021
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