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

config: Improve input checking in config command #209

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

jerry-skydio
Copy link
Collaborator

@jerry-skydio jerry-skydio commented Dec 30, 2024

Previously you could use config to set an invalid value, which would
prevent you from ever running config again to fix it.

  • Run config before loading the parsed config into args. This ensures
    that invalid config files won't prevent running config, and config itself can't
    be configured anyway.

  • Add a --delete option to config to delete keys

  • Validate that the given config command and option are actually valid. if invalid,
    suggest a list of valid options.

  • Error out if the provided value is invalid for the given option, and also add
    choices to the validity check.

Topic: improveconfig
Reviewers: aaron, brian-k

Previously you could use config to set an invalid value, which would
prevent you from ever running config again to fix it.

- Run config before loading the parsed config into args. This ensures
that invalid config files won't prevent running config, and config itself can't
be configured anyway.

- Add a --delete option to config to delete keys

- Validate that the given config command and option are actually valid. if invalid,
suggest a list of valid options.

- Error out if the provided value is invalid for the given option, and also add
choices to the validity check.

Topic: improveconfig
Reviewers: aaron, brian-k
@jerry-skydio
Copy link
Collaborator Author

jerry-skydio commented Dec 30, 2024

Reviews in this chain:
#209 config: Improve input checking in config command

@jerry-skydio
Copy link
Collaborator Author

jerry-skydio commented Dec 30, 2024

# head base diff date summary
0 2c402d32 159f964c diff Dec 30 17:46 PM 3 files changed, 107 insertions(+), 75 deletions(-)
1 eea3ccd2 159f964c diff Dec 30 20:05 PM 2 files changed, 47 insertions(+), 28 deletions(-)
2 cce56da6 159f964c diff Dec 30 20:12 PM 1 file changed, 6 insertions(+), 2 deletions(-)
3 59bcdde4 159f964c diff Dec 30 20:28 PM 2 files changed, 19 insertions(+), 5 deletions(-)
4 1bb71de9 159f964c diff Dec 30 20:30 PM 1 file changed, 1 insertion(+), 1 deletion(-)

@jerry-skydio jerry-skydio force-pushed the jerry/revup/main/improveconfig branch from 2c402d3 to eea3ccd Compare December 31, 2024 01:05
@jerry-skydio jerry-skydio changed the title config: Fix chicken/egg scenario with config config: Improve input checking in config command Dec 31, 2024
@jerry-skydio jerry-skydio force-pushed the jerry/revup/main/improveconfig branch 2 times, most recently from cce56da to 59bcdde Compare December 31, 2024 01:28
@jerry-skydio jerry-skydio force-pushed the jerry/revup/main/improveconfig branch from 59bcdde to 1bb71de Compare December 31, 2024 01:30
Copy link
Contributor

@aaron-skydio aaron-skydio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice

Comment on lines +29 to +30
elif option_string.startswith("-"):
no_options.append("-n" + option_string[1:])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this idiomatic? I tend to think short cli args can be stacked like tool -abcdef, and yeah that's how argparse works also: https://docs.python.org/3/library/argparse.html#option-value-syntax

It seems like it'd be confusing to pass e.g. -nx and not know if that's short for -n -x or --no-xlong-argument, I think the way revup is set up here it'd always be --no-xlong-argument but I'm not sure if I've seen a cli tool where that'd be the case

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah true, fwiw we don't have any flags that use -n for roughly this reason (except stragglers in toolkit that can be changed) would this be better if we formalized that?

we can also get rid of the short syntax and use only --no- longform syntax, i'm not that attached to it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd rather use only --no- longform

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg i'll make that a new pr though, its not strictly related to this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@jerry-skydio jerry-skydio merged commit e9c1d3b into main Jan 2, 2025
5 checks passed
@jerry-skydio jerry-skydio deleted the jerry/revup/main/improveconfig branch January 2, 2025 18:41
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