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

DM-24979: config_file_option does not save the 'help' input argument #286

Merged
merged 6 commits into from May 21, 2020

Conversation

n8pease
Copy link
Contributor

@n8pease n8pease commented May 19, 2020

No description provided.

@@ -24,11 +24,15 @@


class config_file_option: # noqa: N801
def __init__(self, required=False, help=None):

defaultHelp = "The path to the config file."
Copy link
Member

Choose a reason for hiding this comment

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

This help doesn't make it clear to reader which type of config this is. Is it the config file for a butler or the config file for a task? Given that the butler config file is only used in butler create (after that you get a choice of specifying the config file or the repo root) we may want to consider it's name more carefully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it always going to be a particular kind of configuration file for any use of --config-file? I guess I'm not clear if shared options should always be absolutely exactly the same, or if there is room for flexibility, and how much of that. create passes its own help text into config_file_option, maybe there shouldn't be a default help and the help arg should be required. I was trying to split the difference, it's possible/likely I haven't found the sweet spot just yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now this option serves two purposes, for repo cfg (butler create) and pex_config in other places. Since we are considering changing the create --config-file option to a different name, this can be made more specific for pex_config files, when we implement that change.

from lsst.daf.butler.cli.opt import config_file_option, config_option, dataset_type_option


class DatasetTypeSuite(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

It's a TestCase not a Suite.

self.assertIn("the dataset type", result.stdout)


class ConfigSuite(unittest.TestCase):
Copy link
Member

Choose a reason for hiding this comment

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

Again TestCase not TestSuite

@n8pease n8pease force-pushed the tickets/DM-24979 branch 2 times, most recently from d91811c to e9c6aca Compare May 20, 2020 21:48
@n8pease n8pease merged commit 2cb96f3 into master May 21, 2020
@timj timj deleted the tickets/DM-24979 branch February 16, 2024 17:18
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.

None yet

2 participants