Support += and -= for multi-value fields in modify#6648
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6648 +/- ##
==========================================
+ Coverage 74.78% 74.82% +0.03%
==========================================
Files 163 163
Lines 20966 20991 +25
Branches 3302 3308 +6
==========================================
+ Hits 15680 15707 +27
+ Misses 4529 4528 -1
+ Partials 757 756 -1
🚀 New features to boost your workflow:
|
|
I really love the feature idea and the straightforward syntax suggestion but don't really have time to look into it these days.... If I'm not mistaken this feature was requested already, maybe even multiple times. Do you mind looking up those issues or did you already and couldn't find them? |
snejus
left a comment
snejus
left a comment
There was a problem hiding this comment.
Well done. I think we'd like ModifyOperation class to be a bit smarter and own all relevant methods, see my comments
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class ModifyOperation: |
There was a problem hiding this comment.
I think this can be simplified to a NamedTuple.
| ) | ||
|
|
||
|
|
||
| def _apply_modify_operation(obj, field, mod, value): |
There was a problem hiding this comment.
I want this to be a method on ModifyOperation, e.g. def apply.
| key, operator = key[:-1], key[-1] | ||
| key = maybe_replace_legacy_field(key, is_album, modify=True) | ||
| mods[key] = val | ||
| mods[key] = ModifyOperation(operator, val) if operator else val |
There was a problem hiding this comment.
Let's only have ModifyOperations here! Simply set operator=None when we have no operator.
|
hi @Victor0700 will you find time to continue on this PR? In case not, I might pick it up and address @snejus suggestions. I can't promise any timeframe though. So what's your status? Anyway, I'm using these features and can't live out them already 😆 Thanks a ton for the submission! |
JOJ0
force-pushed
the
modify-multivalue-operators
branch
from
910fa5f to
bc3f456
Compare
|
@snejus and @wanxidong I'm taking over here with addressing the suggested code changes. Those are done and ready for a review. Will provide docs later. HTH :-) |
|
@JOJ0 I love your keenness! I actually want these features as well. |
| from .utils import do_query | ||
|
|
||
|
|
||
| class ModifyOperation(NamedTuple): |
There was a problem hiding this comment.
Each of the new functions and ModifyOperation.apply need types here
| obj_mods[key] = ( | ||
| mod.apply(obj[key], parsed_value) | ||
| if mod.operator is not None | ||
| else parsed_value | ||
| ) |
There was a problem hiding this comment.
ModifyOperation now handles the case when operation is None:
| obj_mods[key] = ( | |
| mod.apply(obj[key], parsed_value) | |
| if mod.operator is not None | |
| else parsed_value | |
| ) | |
| obj_mods[key] = mod.apply(obj[key], parsed_value) |
| item.load() | ||
| assert item.title == f"{orig_title} - append" | ||
|
|
||
| def test_modify_multi_value_assignment(self): |
There was a problem hiding this comment.
All these tests (except test_modify_scalar_operator_error) should be used with @pytest.mark.parametrize.
Just define a new class TestMultiValue(TestHelper),
define a @pytest.fixture for item and merge them into a single parametrized test. See test_sort.py for some examples.
3 participants
Description
Fixes #6587.
This PR adds
+=and-=support tobeet modifyfor multi-value fields.For example:
With this change,
genres+=FunkappendsFunkto the existinggenresvalues if it is not already present, whilegenres-=Bluesremoves only the exact valueBluesand preserves other values such asBlues Rock.The existing
field=valuereplacement behavior is preserved.Main behavior changes:
genres=Jazz; Bluesstill replaces the full field value as before.genres+=FunkappendsFunkto the existing multi-value field.genres+=Funkdoes not create duplicate values.genres-=Bluesremoves only the exact valueBlues.Bluesdoes not remove partial matches likeBlues Rock.+=and-=are rejected for scalar fields with a clear user-facing error.To Do
modifycommand documentation if maintainers prefer.Verification
Ran: