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

Default command-line option parsing to override configured defaults #13

Closed
pwdirks opened this issue Apr 27, 2020 · 9 comments
Closed
Assignees
Labels
Common Config Common configuration (port, speed, type, etc.) related depends on... Depends on another issue being resolved enhancement New feature or request

Comments

@pwdirks
Copy link
Collaborator

pwdirks commented Apr 27, 2020

The new 'config' module enables consistent configuration of various settings but we might consider some standard code to parse command line options for overrides of all of the configurable options, including ones suggested in Issue #12?

@AESilky
Copy link
Collaborator

AESilky commented Apr 27, 2020

I agree. The code in Configure.py could be pulled out into a library so it isn't repeated and the '-' and '--' values used across all programs/commands are consistent. The non-trivial part will be a way to indicate which of the options are valid for each individual program/command. For example - in 'Sample.py', the current three options are the only ones that are applicable, so being able to specify any of the other options (as suggested in #12) doesn't make sense.

@AESilky
Copy link
Collaborator

AESilky commented Apr 27, 2020

This should depend on #14 to avoid duplicating option processing code and make sure all options are consistent across all applications/commands.

@AESilky
Copy link
Collaborator

AESilky commented Apr 27, 2020

Depends #14

@AESilky AESilky added the depends on... Depends on another issue being resolved label Apr 30, 2020
@pwdirks
Copy link
Collaborator Author

pwdirks commented May 1, 2020

If we agree on using the standard We could either argument parser from Lib/argparse.py, ArgumentParser, we can (either in config.py or in a new args.py, which seems cleaner) define argparse.ArgumentParser objects for each of the command line options (e.g. "--wire", "--remote, "--wpm"). If we define objects for every option then each individual program can string together whichever ones it wants to use with the 'parents' attribute (see https://docs.python.org/3/library/argparse.html#parents) in a parser they construct at init time:

import argparse
clock_parser = argparse.ArgumentParser()
clock_parser = argparse.ArgumentParser(parents=[WIRE_PARSER, WPM_PARSER])

and then invoke

args = clock_parser.parse_args()

to collect any overrides for the command line options that are meaningful to the particular program, still giving appropriate 'usage' and 'help' messages? That would also leave some commands free to add their own unique options (e.g. "--interval=10" to announce every 10 minutes)?

@AESilky
Copy link
Collaborator

AESilky commented May 2, 2020

I like that approach. We just need to make sure we document it well (provide a good example), so someone writing a new application doesn't have a problem using it.

@AESilky
Copy link
Collaborator

AESilky commented May 2, 2020

I think we should pursue this approach rather than what I put in #14.

@AESilky
Copy link
Collaborator

AESilky commented May 25, 2020

I don't want to lose track of this when #12 is closed and I think the approach is what we need to discuss per the comments in #12.

I agree that pulling command line option processing (of any type - even 'overrides') out of Config.py is a good thing. I don't suggest doing that as part of #12. I would like to get #12 wrapped up so it can be merged to master and we can excercise it to work out bugs and consider improvements. Then we can address this issue to improve the current implementation. There currently aren't too many apps to make changes to.

However, we need to keep in mind that this repo IS public, so people could be writing apps based on this code. I don't think anyone is (yet), but something to keep in mind for the future.

@AESilky AESilky added the Common Config Common configuration (port, speed, type, etc.) related label May 25, 2020
@pwdirks
Copy link
Collaborator Author

pwdirks commented May 25, 2020

I agree it's fine to merge #12's change as-is and address this separately. Please go ahead and merge into 'master' so we don't hit more conflicts with this outstanding change.

We can use this issue to create suitable overrides in various programs after the merge.

@AESilky
Copy link
Collaborator

AESilky commented May 30, 2024

Closing this, as the Config class in config2.py (created to allow named configurations) provides methods to process all the 'standard' configuration options and create a Config instance from them. The individual application/utility is still in control of what options to make available (by including the 'overrides' to an argparse instance), but then the args that result from parsing are passed to the process_config_args method which sets the values on a Config instance.

In addition to the standard config option processing, there is also (now) a pkappargs.py module that handles the additional options that are common to the MKOB and MRT applications.

pykob version 1.3.4

@AESilky AESilky closed this as completed May 30, 2024
@AESilky AESilky self-assigned this May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Common Config Common configuration (port, speed, type, etc.) related depends on... Depends on another issue being resolved enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

2 participants