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-46297: require label to be provided to write-curated-calibrations #490

Merged
merged 4 commits into from
Sep 19, 2024

Conversation

TallJimbo
Copy link
Member

@TallJimbo TallJimbo commented Sep 13, 2024

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

"labels_arg",
metavar="LABELS",
nargs=-1,
help="One or more extra strings to include in the collection name (see --label).",
Copy link
Contributor

@leeskelvin leeskelvin Sep 18, 2024

Choose a reason for hiding this comment

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

The CLI help looks like this now:

$ butler write-curated-calibrations --help
Usage: butler write-curated-calibrations [OPTIONS] REPO INSTRUMENT LAB ...

  Add an instrument's curated calibrations to the data repository.

  REPO is the URI or path to the gen3 repository. Will be created if it does
  not already exist

  The name or fully-qualified class name of an instrument.

  One or more extra strings to include in the collection name (see --label).

I guess LAB has been auto-truncated from LABEL?

I think my main concern here however is that both INSTRUMENT and LAB/LABEL are not specifically named prior to their description. This wasn't so bad when there was only REPO and INSTRUMENT, as REPO is labelled, so it's obvious what the other docstring is referring to. However, now that we've added another, I think it would be helpful to prefix the docstring here with "LABEL is one or more...".

It may be out-of-scope to add "INSTRUMENT is..." on this ticket, but that would probably help with consistency too.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was actually my first encounter with the mechanism for how the positional argument docs are assembled, which seems to be a workaround for the fact that Click just expects you to put the docs for position arguments into the main usage string yourself. It seems we've been using that workaround incorrectly basically forever, and I think it might have been better to have never invented it (yes, there would then be some duplication, but for docstrings in particular I think it might not be the bad kind of duplication), and given how carefully-considered much of Click is, I bet that's what the design intent (which we've apparently subverted) is.

Ok, enough ranting:

  • I'll rename LABEL for the positional arg to LBL; I don't think I can stop the shortening (which is I assume in Click itself) any other way.
  • I'll add a LBL: prefix to the sentence about the new positional argument pretty easily.
  • Modifying the other positional-argument docstrings would require changes in daf_butler and/or pipe_base, and I don't really want to make more branches for this ticket just for that. I'd prefer to walk back that workaround and have our scripts start documenting their positional arguments as Click intended.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Jim, that change makes sense to me. I agree it's probably too much to modify daf_butler / pipe_base on this ticket, and I think naming LBL will remove any potential confusion in any case. Thanks also for the DRY link - an interesting read!

@TallJimbo TallJimbo merged commit 54c5bc6 into main Sep 19, 2024
6 checks passed
@TallJimbo TallJimbo deleted the tickets/DM-46297 branch September 19, 2024 16:19
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