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

Only include non default arguments when invoking process #777

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jdavidberger
Copy link

This is a WIP pull request; it does NOT pass unit tests and would need a bit more work to be mergeable. The main question right now is if its a feature that would be mergeable in general.

The basic issue I have is that Gooey passes even default options to the process; and so replicating a minimal command line equivalent is hard.

Since this change would change current behavior, I'm thinking it'd need to be gated behind a default off buildspec argument. If that is something that you'd merge in, I'll add that option in and propagate it through.

@chriskiehl
Copy link
Owner

Howdy!

Thanks for checking in and giving a preview of what you're wanting to add!

As a quick heads up, Gooey's internals are currently being reworked super heavily, so a PR will (a) possibly be targeting outdated code, and (b) likely to be waiting awhile. So, I'd suggest holding off for a bit. That said, the two files you're changing are, so far, not being modified on my end ^_^

For the feature, could you give a bit more background on what you're trying to do with this change? Because of how argparse defaults work, whether or not they're sent along results in the same end-state. e.g.

parse_args("--arg1 value") == parse_args("--arg1 value --arg2 some_default") == Args(arg1=value, arg2=some_default)

Based on that, to my mind their inclusion is almost irrelevant unless you're actually looking at the string. So, when you say "replicating a minimal command line", are you trying to use Gooey just for generating the command line string itself (for use manually), rather than having it invoke the actual code or something?

Is this primarily targeting aesthetics of the cli string?

@jdavidberger
Copy link
Author

Is this primarily targeting aesthetics of the cli string?

More or less yes. Right now the tool I'm targeting is something where I change a few parameters, run, look at the results, and iterate. Once I get a set of parameters I find interesting, I want to be able to note that down / rerun it later with new code. Up until recently, I've done this just on the CLI. Using gooey is nicer in terms of changing parameters and the like, but being able to copy off the ran CLI, and close gooey, change some code, and then rerun the tool with those changed parameters is an important part of that workflow. And since the tool has more than 50 parameters, copying them all around isn't super clean.

A somewhat related use case is I'll occasionally send people CLI snippets to run or put them in a readme and so a lot of default arguments there gets unwieldy. (As a sidenote; I also thought of making a PR for having a 'copy CLI' button; but don't know enough WX to make that pretty; so I just print it out in my program)

So I don't think these are the most common use cases for this tool; but I don't think they are completely unique either. Since the argparse state ends up the same either way (which I agree it should), the buildspec option is maybe unnecessary but having the behavior be opt-in would be 100% sure not to break existing users.

@chriskiehl
Copy link
Owner

Righto. Makes sense! Thanks for the extra info.

It sounds totally reasonable to me. Enabling it via flag is a good choice. Lemme get out the next release (cause, as mentioned, it's currently mangling a ton of internals), but then I'll happily accept a PR.

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