Study version display #289
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
cumulus_library/cli.py
Outdated
| if target in ("core", "discovery", "vocab"): | ||
| print( | ||
| f"{target} distributed with Cumulus library.\n" | ||
| f"Cumulus library: {__version__}" | ||
| ) |
There was a problem hiding this comment.
I'm a little leery of this special logic here. The user can point at a different / in-development version of these built-in studies with -s, where this may not give the right version. And vocab is slated for movement - so depending on how that goes down, old versions of library may not give the correct version after the move, especially if -s is involved again.
I understand you're doing that so that you don't have to keep track of versions inside core/discovery/vocab. But also... maybe we should? Like, discovery has its own semantic version lifecycle separate from core, which in theory lives a different versioning life from the outer library study runner (though in practice, those two are often closely entwined).
Anyway, just a nit comment, this is fine for now.
There was a problem hiding this comment.
i could make an argument for moving discovery to its own repo, and removing vocab, which would make this repo just the infrastructure and the core study).
I could :also: make an argument for moving the core study to its own repo, though it complicates the installation story a bit.
There was a problem hiding this comment.
Well I wasn't even talking about repos - just a version in their __init__.py files
There was a problem hiding this comment.
But the separate-version approach also matters less if we do a "all versions printed" approach from below.
There was a problem hiding this comment.
this was sort of a side thought - i'm doing the all versions thing below for now, i guess i'm going to pin core to a v3 since it's mostly tied to the core infra and discovery to 1.0, but i could see a decoupling case generally? not taking action on it atm.
cumulus_library/cli_parser.py
Outdated
| @@ -297,4 +297,10 @@ def get_parser() -> argparse.ArgumentParser: | |||
| add_target_argument(markdown) | |||
| add_verbose_argument(markdown) | |||
|
|
|||
| # Get study version | |||
|
|
|||
| study_version = actions.add_parser("study-version", help="Gets the version of a specific study") | |||
There was a problem hiding this comment.
This is fine. But I would vote for a single command that shows all known versions it can find. Like... if --version (and/or a version or debug subcommand) gave output like:
$ cumulus-library --version
Cumulus Library: 3.0.0
Python: 3.11.1
Studies:
covid_symptom: 2.1.0
...etc...
umls: 1.2.0
Since we register known studies in the Library, we can just iterate over that (plus any extra targets). You could throw in paths to the study modules if you wanted there too.
In my mind, this has some advantages:
- Easier to tell a user how to do it: "run --version" vs "run study-version -t covid_symptom -t core"
- Less error prone for the user
- harder to forget a
-t coreor similar argument that we'd want to see, everything is built-in - or a situation where they run a
study-version -t covid_symptomline they saved from us a week ago instead of the newstudy-version -t umlscommand we just gave them.
- harder to forget a
- Easier to add future useful bits, like the version of some major dependency like Jinja or whatever.
But this approach works too - I'm just brainstorming.
There was a problem hiding this comment.
i buy this (as long as I alpha sort the keys for lookup)
There was a problem hiding this comment.
ok, this is now the behavior.
There was a problem hiding this comment.
While doing this versioning init pass - I'm pulling this since we're no longer doing cli template creation.
cumulus_library/cli_parser.py
Outdated
| @@ -297,4 +297,10 @@ def get_parser() -> argparse.ArgumentParser: | |||
| add_target_argument(markdown) | |||
| add_verbose_argument(markdown) | |||
|
|
|||
| # Get study version | |||
|
|
|||
| study_version = actions.add_parser("study-version", help="Gets the version of a specific study") | |||
There was a problem hiding this comment.
ok, this is now the behavior.
This PR adds the
--study-versionarg, for reading the version of a given study from it's init.py. #243Also updated the docs to make this new init.py requirement to the study authoring docs.
Checklist
docs/needs to be updatedgenerate-mdcorestudy fields that not in US Core, update our list of those incore-study-details.mdmanifest.toml