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

add unknown flags support #285

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TiboStev
Copy link

Make able to use unknown flag and make it accessible for future use. For a wrapper for example

@CLAassistant
Copy link

CLAassistant commented Sep 18, 2020

CLA assistant check
All committers have signed the CLA.

@TiboStev TiboStev force-pushed the add_visitUnknows_capability branch from 4f9eec8 to 33dec6a Compare September 18, 2020 20:45
@cornfeedhobo
Copy link

There have been a few PRs related to the idea of tracking unknown fags. Can you explain why you chose this approach over #199?

@TiboStev
Copy link
Author

I didn't see #199, I fell that using flags and VisitUnknowns() may fit better the API and make it more elegant if one needs to iterate over unknown flags. But in the end I just need a way to retrieve unknown flags so one way or another I'm fine with it.

@cornfeedhobo
Copy link

cornfeedhobo commented Sep 20, 2020

@TiboStev No worries. Design decisions are tough, that's why I had to ask. I've gone ahead and implemented #199 on my fork, but I like this branch because it's got a similar API and the changes to parsing are much more clear for the reader.

@cornfeedhobo
Copy link

@TiboStev Made a version of this on my fork. Feel free to check it out 😃

@TiboStev
Copy link
Author

TiboStev commented Oct 9, 2020

thanks for the review @cornfeedhobo I'll have a look

@kennytm
Copy link

kennytm commented Nov 30, 2020

There is also #187 besides #199 🤷

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.

4 participants