DM-33414: Allow id to be used in file template #639
Conversation
timj
force-pushed
the
tickets/DM-33414
branch
4 times, most recently
from
0f631e2
to
be21926
Compare
Codecov Report
@@ Coverage Diff @@
## main #639 +/- ##
==========================================
+ Coverage 84.13% 84.14% +0.01%
==========================================
Files 237 237
Lines 30216 30223 +7
Branches 5022 5022
==========================================
+ Hits 25423 25432 +9
+ Misses 3648 3647 -1
+ Partials 1145 1144 -1
Continue to review full report at Codecov.
|
| if not usedRun and not usedId: | ||
| missing = ("run" if not usedRun else None, "id" if not usedId else None) | ||
| text = " or ".join(f"'{m}'" for m in missing if m is not None) | ||
| raise KeyError(f"Template does not include {text}.") |
There was a problem hiding this comment.
Is there a similar check for the dataset type name that could also be relaxed?
There was a problem hiding this comment.
It doesn't look like we require the dataset type to be in the template. "run" was the only thing we required from what I could see (and I was able to use a file template of just {id}.
There was a problem hiding this comment.
The fact that this line has no coverage implies that we catch the no-run logic higher up and none of this is needed any more.
There was a problem hiding this comment.
Sounds like we should just remove it, then, and update the docs to say it's expected?
There was a problem hiding this comment.
I think that this block of code was an early attempt at template validation but at some point we moved the validation logic earlier and never noticed that this block was irrelevant. I'll remove it.
timj
force-pushed
the
tickets/DM-33414
branch
from
be21926
to
9d27818
Compare
There was a problem hiding this comment.
Seems fine - functionality is clearly what we want, but I'm a little worried that the template validation code is getting a little hard to follow now that we have so many rules (especially if we don't really understand why some of the new lines aren't in the coverage report).
If id is used it must at least be present in the file name part of the template path.
During DM-17025 the code in the format() method to check that run was present was made irrelevant because of an earlier check that was added as part of generic template validation.
timj
force-pushed
the
tickets/DM-33414
branch
from
9d27818
to
aa2e8a4
Compare
@gpdf / @TallJimbo this does the job -- I also restrict the id to having to be in the filename itself if it is specified anywhere.
Checklist
doc/changes