-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store albumtypes
multi-value field consistently in-DB & in-tag, preventing continual file re-tagging
#4582
Merged
Commits on Feb 27, 2023
-
Configuration menu - View commit details
-
Copy full SHA for 41f9ecc - Browse repository at this point
Copy the full SHA 41f9eccView commit details -
Configuration menu - View commit details
-
Copy full SHA for af65c6d - Browse repository at this point
Copy the full SHA af65c6dView commit details -
Realign with known-working code after review by @mkhl
@mkhl was kind enough to do a drive-by review of my proposed changes, which I'll include here as the GitHub URI may bit-rot over time (it's technically [here](beetbox@bc21caa), but that commit isn't part of the `beets` repo, so may get GC'd). I've encorporated all their proposed changes, as their code is being run against an existing Beets library, whereas my changes were made as I tried to set up Beets for the first time - thus I'm inclined to trust their known-working code more than my own! This is a review starting at beetbox@bc21caa#diff-d53f73df7f26990645e7bdac865ef86a52b67bafc6fe6ad69890b510a57e2955R210 (`class DelimeteredString(String):`) > for context this is the version i'm using now: > > ```python > class DelimitedString(String): > model_type = list > > def __init__(self, delimiter): > self.delimiter = delimiter > > def format(self, value): > return self.delimiter.join(value) > > def parse(self, string): > if not string: > return [] > return string.split(self.delimiter) > > def to_sql(self, model_value): > return self.delimiter.join(model_value) > ``` > > i think 'delimited string' is the correct term here > > the rest of the code doesn't seem to use many abbreviations, so calling the property `delimiter` seems appropriate > > i don't think a default value for the delimiter makes a lot of sense? > > the list comprehension and string conversions in `to_sql` don't seem necessary to me, see above. did you run into trouble without them? > > the `from_sql` seems to just be missing functionality from the `Type` parent and seems completely unnecessary > > `parse` shouldn't be able to fail because at that point, we've ensured that its argument is actually a string. i also added a `if not string` condition because otherwise the empty list of album types would turn into the list containing the empty string (because that's what split returns) > > if we don't define a `format` method here we print the internal python representation of the values (i.e. `['album', 'live']` or somesuch) in the `beet write` output. joining on the delimiter nicely formats the output :) > > just so i don't ping you twice unnecessarily, i think it's better to instantiate this type with `'; '` (semicolon space) as the delimiter, because that's what was used before to join the albumtypes and what we'll find in the database All these changes have been made, including the switch from `;` to `;<space>` as the in-DB separator.
Configuration menu - View commit details
-
Copy full SHA for 7cfc39e - Browse repository at this point
Copy the full SHA 7cfc39eView commit details -
Configuration menu - View commit details
-
Copy full SHA for cd52a05 - Browse repository at this point
Copy the full SHA cd52a05View commit details -
Configuration menu - View commit details
-
Copy full SHA for 27218a9 - Browse repository at this point
Copy the full SHA 27218a9View commit details -
Fix albumtypes plugin and its tests
The new database type DelimitedString does list to string and vice versa conversions itself.
Configuration menu - View commit details
-
Copy full SHA for 93fa19f - Browse repository at this point
Copy the full SHA 93fa19fView commit details
Commits on Feb 28, 2023
-
Rewrite changelog entry for beetbox#4583
and include linking to manual fixing tutorial.
Configuration menu - View commit details
-
Copy full SHA for 7be1eec - Browse repository at this point
Copy the full SHA 7be1eecView commit details -
Add note to albumtypes plugin docs about beetbox#4528
Add a note to the docs of the albumtypes plugin warning about issue beetbox#4528 and linking to the manual fixing description.
Configuration menu - View commit details
-
Copy full SHA for 78853cc - Browse repository at this point
Copy the full SHA 78853ccView commit details -
Configuration menu - View commit details
-
Copy full SHA for 4908e1a - Browse repository at this point
Copy the full SHA 4908e1aView commit details
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.