-
-
Notifications
You must be signed in to change notification settings - Fork 5
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 parser for flatpak
and flatpak update
#104
base: master
Are you sure you want to change the base?
Conversation
Parsers serve two purposes: 1. They determine whether or not a command executes its arguments. 2. They determine which parts of a command’s arguments are other commands that get executed. Before this change, parsers would have to do both, or else you would get an AttributeError. Now, you can create a partial parser that can confirm that a specific invocation doesn’t run any of its arguments but can’t handle invocations that do run their arguments. Specifically, this change is in preparation for adding a parser for flatpak. flatpak has 43 built-in subcommands, some of which are able to execute their arguments. I don’t want to bother adding a parser that supports all 43 of those. I want to add a parser that supports a single subcommand, and that subcommand can’t run its arguments.
This parser doesn’t cover all of flatpak’s subcommands because I don’t use all of flatpak’s subcommands in my own scripts. I created this script in order to help me test out this change: <https://gist.github.com/Jayman2000/35a6db532e1fd718d2039c36bb25ed89>
I have some WIP fixes that are making incremental progress on this, but as I'm working through it I'm thinking I should double-check my understanding. Generally speaking, I've only been adding parsers to help resholve find the executable argument in the haystack. I don't see one in the arguments here, and since you mention issue 90, I'm guessing this means that:
|
Yep.
I’m not sure. I know that
Yep.
Exactly. I’m also interested in submitting another pull request that does the same thing for some of Git’s subcommands. |
Hehe. Fair. I'll chew on this. Thanks for the confirming. It doesn't square with how I've been using the parsers, but I agree there's some merit. I've avoided dealing with git for similar reasons. (I've also dragged my feet a little because I'm not happy with the parsers. And I'd like to figure out how to document these human-review decisions in a way that pins to real source and generates assertions we can run in CI that won't light up on every update but will light up when they add new arguments... some way to see when the knowledge they encode is rotting.) At least in the face of the problem posed by some of these massive subcommand CLIs, I guess it would be nice to have a way to just dead-end an entire subcommand without having to implement arguments if we've checked it for exec and don't see any. I'll play around with argparse and see if I can figure something out. |
Curious what you think about the approach (and perhaps more importantly the right term--I suspect dead-end encodes too much of my perspective) here: 914061c#diff-3791dcc7805ec6044570664bd7360fb4e9441c5987f7df8641b50e8bdac2df48L2421-R2436 Basically, adding support treating a command as handled if we encounter a ~special |
Here are my notes:
|
Thanks for noticing. I intended both the item and the final bracket on their own lines, but that column in my editor was a bit too narrow and the line-wrap made it look right.
Good catch. I've felt all along like my (the?) language around these executable arguments is a bit slippery. A lot of the most-common terms are very overloaded already, and then Shell helps weird things further by layering in concepts like command substitution. Most of what we care about is external executables executing other user-supplied external executables, but down at the parser layer it also has to close over builtins, some of which can execute more than just external executables. What about:
Good point.
(I'm switching up the order, because knowledge of this argparse quirk will play a minor role in the last note :) Thanks for poking at this. I had seen it, but it didn't set off my ~detector. Looks like a known issue with subparsers: https://bugs.python.org/issue9253. TL;DR: subparser argument is effectively required, and invocations without it present cause the "too few arguments" error. I don't want to completely discard the message since it might still be a breadcrumb when it comes to getting one of these parsers working, but I guess we can just selectively downgrade the level to debug when there's a subparser present...
Thanks for pointing out the failing invocation. I'm not really decided yet, so I'll just think out loud here (I'm not looking for a response, but feel free). I patched it out to see if I can satisfy your use-case without it. It might end up being the best option, but I have some concerns about it that I need to either address or convince myself are non-issues:
I need to chew on it a bit more, but basically: I suspect I should try to make this behavior an explicit affirmative property of specific parsers and/or options in a way that clarifies the fundamental difference between the assertions they represent. |
Just a ping to let you know I haven't forgotten this and thank you again for kicking it off. I agree with carving out space for this kind of parser and know roughly how I want to approach that, but I held up a little to mull naming. Life/work/other-projects keep conspiring to demand time, but I think (hope?) this is close to the top of the pile again. |
Ok. I've had a chance to take another swing at this and cover most of your notes in the process. c3d8bab This separates these two broad approaches into different parser subclasses. I'm still not certain about the names. I'm also not sure how the deadend subparser should fit in here now; I haven't tried to pick at that behavior much. |
OK, so I just took a look at c3d8bab. Here are my notes:
Could you elaborate? It looks to me like subparsers fit in fine at the moment. |
I still need to take a closer look at this to make sure I'm both understanding and not mis-remembering, but I think I recall stumbling on an issue report (I think I had the tab open until a tab purge a few weeks back...) about argparse with a subparser throwing this error if invoked without at least one positional argument. I'll try to find that again in my history and add some of these example invocations to the flatpak parse test file and see if there's anything we can do. Do you happen to have a public example I can mine for legit invocations that shouldn't error out?
Good point. Now that you poke at it, I realize I'm leaving a lot unsaid about what a variant is in the first place. I'll probably squash them later, but for now I've just added another commit (49b76e8) to actually unpack why "variant" is a concept here. Do you feel like that works?
I'll stew on these. (At first blush I have a very small qualm with audit here, but I also see the argument that it's closer to to the human-level ~why behind the different parsers. Have to talk myself around it a bit...)
I'm not exactly sure what I mean, yet. It's not a concern about subparsers broadly--just the deadend type. IIRC, it was vague handwringing about some bits I wasn't motivated to brain into submission at the time. This may not be a complete list, but something like:
|
Not really. The original reason why I had started implementing a parser for
Yeah, that commit makes things much clearer. |
I've revisited this today to see if I could settle the naming issue and finish out the WIP I had here, but it hasn't gone so well. I found the argparse thread I mentioned before, and it also isn't great news: python/cpython#103520 (comment)
I'm now realizing there's a conceptual trap with trying to use the partial parsers--since they have to fail everything they can't match, there's no sane way to ~chain them for slightly different purposes as with the full parsers. (This might only be a problem because of this argparse shortcoming, though.) I'm a little unsure what to do with this case, now. I think we'd have to either patch up argparse somehow (but the quote above makes me think that'll also have its problems), or maybe concede defeat and either patch up this single parser to work around the limitation or build in a custom flatpak parser. I'm not certain, but those both smell like they might waste more time than just auditing flatpak? Unless I find a lightbulb moment, I'm starting to think this was a dead-end in a no-pun-intended sort of way. I'm a little bummed, but don't take this as me being down on you for bringing it up. I'm glad you did--I probably would've had to go down this path sooner or later just to convince myself it wasn't this easy. Some other silver linings are:
|
I stumbled on some links I saved relating to the argparse bit of this, suggesting that this got better in python3.7: |
This PR only implements a parser for
flatpak
and one of its subcommands. I think that #90 would have to be dealt with before a parser that supports all offlatpak
’s subcommands and arguments could be implemented.I’m submitting this as a draft because always crashes at the moment:
It looks like
CommandParser
doesn’t support all of the arguments thatArgumentParser
does. Adding a subparser causes a newCommandParser
to get created, and that causes the unexpected keyword argument error. I tried adding additional keyword arguments toCommandParser.__init__()
, but then I ran into more errors, and I’m still not sure if that’s the right solution to this problem.