mirror of
https://github.com/beetbox/beets.git
synced 2026-01-03 22:42:44 +01:00
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](bc21caa0d5), 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 atbc21caa0d5 (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.
This commit is contained in:
parent
af65c6d707
commit
7cfc39ea27
2 changed files with 16 additions and 18 deletions
|
|
@ -207,29 +207,27 @@ class String(Type):
|
|||
else:
|
||||
return self.model_type(value)
|
||||
|
||||
class DelimeteredString(String):
|
||||
|
||||
class DelimitedString(String):
|
||||
"""A list of Unicode strings, represented in-database by a single string
|
||||
containing delimiter-separated values.
|
||||
"""
|
||||
model_type = list
|
||||
|
||||
def __init__(self, delim=','):
|
||||
self.delim = delim
|
||||
def __init__(self, delimiter):
|
||||
self.delimiter = delimiter
|
||||
|
||||
def to_sql(self, model_value):
|
||||
return self.delim.join([str(elem) for elem in model_value])
|
||||
|
||||
def from_sql(self, sql_value):
|
||||
if sql_value is None:
|
||||
return self.null()
|
||||
else:
|
||||
return self.parse(sql_value)
|
||||
def format(self, value):
|
||||
return self.delimiter.join(value)
|
||||
|
||||
def parse(self, string):
|
||||
try:
|
||||
return string.split(self.delim)
|
||||
except:
|
||||
return self.null
|
||||
if not string:
|
||||
return []
|
||||
return string.split(self.delimiter)
|
||||
|
||||
def to_sql(self, model_value):
|
||||
return self.delimiter.join(model_value)
|
||||
|
||||
|
||||
class Boolean(Type):
|
||||
"""A boolean type.
|
||||
|
|
@ -254,4 +252,4 @@ FLOAT = Float()
|
|||
NULL_FLOAT = NullFloat()
|
||||
STRING = String()
|
||||
BOOLEAN = Boolean()
|
||||
SEMICOLON_DSV = DelimeteredString(delim=';')
|
||||
SEMICOLON_SPACE_DSV = DelimitedString(delimiter='; ')
|
||||
|
|
|
|||
|
|
@ -504,7 +504,7 @@ class Item(LibModel):
|
|||
'mb_releasetrackid': types.STRING,
|
||||
'trackdisambig': types.STRING,
|
||||
'albumtype': types.STRING,
|
||||
'albumtypes': types.SEMICOLON_DSV,
|
||||
'albumtypes': types.SEMICOLON_SPACE_DSV,
|
||||
'label': types.STRING,
|
||||
'acoustid_fingerprint': types.STRING,
|
||||
'acoustid_id': types.STRING,
|
||||
|
|
@ -1064,7 +1064,7 @@ class Album(LibModel):
|
|||
'mb_albumid': types.STRING,
|
||||
'mb_albumartistid': types.STRING,
|
||||
'albumtype': types.STRING,
|
||||
'albumtypes': types.SEMICOLON_DSV,
|
||||
'albumtypes': types.SEMICOLON_SPACE_DSV,
|
||||
'label': types.STRING,
|
||||
'mb_releasegroupid': types.STRING,
|
||||
'asin': types.STRING,
|
||||
|
|
|
|||
Loading…
Reference in a new issue