verdi code setup: validate the uniqueness of label for local codes
#5215
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #5215 +/- ##
===========================================
+ Coverage 81.23% 81.23% +0.01%
===========================================
Files 533 533
Lines 37345 37356 +11
===========================================
+ Hits 30332 30343 +11
Misses 7013 7013
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks good to me! The only thing I would add is some tests monkeypatching load_code to raise exceptions.MultipleObjectsError just for coverage completeness. Also, more up to you, but I think it is better, since we already know it is a MultipleObjectsError exception, to mention this in the message (f'multiple copies of the code `{value}` already exist.').
|
One side comment that I've just notice is that I think we are not catching "@" in the user input, no? Couldn't this cause problems inside of (EDIT: Wooops wrong ping) |
|
@janssenhenning did you want to take a look at this before we merge? We could probably wait some days if you wanted to check this but don't have time right now, otherwise once my comments are addressed by @sphuber we would probably merge this by the end of the week. Let us know. |
In commit d25339d `verdi code setup` was improved to have a callback for the `label` option to check for its uniqueness. However, it only implemented this for "remote" computers, which have an associated `Computer` and so the uniqueness criterion is on the "full label", which is the `label@computer.label`. However, there are also "local" codes, which don't have an associated computer and so only have the label as the identifier that should be unique. The callback `validate_label_uniqueness` is now updated to distinguish between these two cases.
sphuber
force-pushed
the
fix/verdi-code-setup-local-uniqueness
branch
from
7285d26 to
f85d029
Compare
|
@ramirezfranciscof thanks for the review. Updated the exception message and added tests. |
|
@ramirezfranciscof Sorry I did not have time so far. The fix looks fine to me |
There was a problem hiding this comment.
Great, thanks @janssenhenning and @sphuber , this can go right in!
In commit d25339d
verdi code setupwas improved to have a callback for the
labeloption to check for itsuniqueness. However, it only implemented this for "remote" computers,
which have an associated
Computerand so the uniqueness criterion ison the "full label", which is the
label@computer.label.However, there are also "local" codes, which don't have an associated
computer and so only have the label as the identifier that should be
unique. The callback
validate_label_uniquenessis now updated todistinguish between these two cases.