Add an advanced dynamic_study_prefix field to manifests. #309
Conversation
mikix
force-pushed
the
mikix/dynamic-prefix
branch
from
4290a50
to
d1f214e
Compare
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
mikix
force-pushed
the
mikix/dynamic-prefix
branch
from
d1f214e
to
8381943
Compare
| c_arg = c_arg.split(":", 2) | ||
| c_arg = c_arg.split(":", 1) |
There was a problem hiding this comment.
I always make this mistake (and might have made this one?) - it's number of splits, not number of parts.
This will allow a study to provide a Python file that accepts study options and generates an appropriate study prefix. It's largely targeted at the data_metrics study, but might have application elsewhere.
mikix
force-pushed
the
mikix/dynamic-prefix
branch
from
8381943
to
761773a
Compare
| if study_prefix is None: | ||
| # This slightly wonky approach will give us the path of the | ||
| # directory of a class extending the CountsBuilder | ||
| study_path = Path(sys.modules[self.__module__].__file__).parent |
There was a problem hiding this comment.
I moved this whole chunk of logic to prepare_queries and just used the existing manifest that it sends down, rather than trying to re-discover the manifest and its options.
There was a problem hiding this comment.
makes sense, i think this pre-dates a lot of that infra.
| else: | ||
| # Add the prefix for them (helpful in dynamic prefix cases where the prefix | ||
| # is not known ahead of time) | ||
| export_table_list.append(f"{self.get_study_prefix()}__{table}") |
There was a problem hiding this comment.
This is a notable change - this allows a study author to do:
[export_config]
export_tables = [
"meta_version",
]
without a prefix.
There was a problem hiding this comment.
Interesting - I don't think we should encourage that, though. We don't have to change the logic, but let's just not recommend it.
|
|
||
| These features are for very narrow and advanced use cases, | ||
| designed for internal project studies (like `core`, `discovery`, or `data_metrics`). |
There was a problem hiding this comment.
nit: i'm not in love with 'internal project studies', though i don't have a better idea off the top of my head.
There was a problem hiding this comment.
Yeah fair, I wasn't sure what phrase to use.
| if study_prefix is None: | ||
| # This slightly wonky approach will give us the path of the | ||
| # directory of a class extending the CountsBuilder | ||
| study_path = Path(sys.modules[self.__module__].__file__).parent |
There was a problem hiding this comment.
makes sense, i think this pre-dates a lot of that infra.
| else: | ||
| # Add the prefix for them (helpful in dynamic prefix cases where the prefix | ||
| # is not known ahead of time) | ||
| export_table_list.append(f"{self.get_study_prefix()}__{table}") |
There was a problem hiding this comment.
Interesting - I don't think we should encourage that, though. We don't have to change the logic, but let's just not recommend it.
This will allow a study to provide a Python file that accepts study options and generates an appropriate study prefix.
It's largely targeted at the data_metrics study, but might have application elsewhere.
Checklist
docs/needs to be updatedgenerate-mdcorestudy fields that not in US Core, update our list of those incore-study-details.mdmanifest.toml