Skip to content
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

new generic TrackInfo and AlbumInfo #2654

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@gabrielhdt
Copy link

commented Aug 13, 2017

TrackInfo and AlbumInfo inheritate from class ItemInfo, which behaves
like a dict and is backward compatible regarding attributes management.
apply_metadata function has been updated consequently, favouring dict
like item access more than attributes. Apply_metadata isn't general yet.
#1547

new generic TrackInfo and AlbumInfo
TrackInfo and AlbumInfo inheritate from class ItemInfo, which behaves
like a dict and is backward compatible regarding atributes management.
apply_metadata function has been updated consequently, favouring dict
like item access more than attributes
@mention-bot

This comment has been minimized.

Copy link

commented Aug 13, 2017

@gabrielhdt, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sampsyo, @jrobeson and @dosoe to be potential reviewers.

gabrielhdt added some commits Aug 13, 2017

Corrections, unit tests successfully passed
Updated calls to TrackInfo and AlbmInfo using keywords only arguments,
implemented reduce methods for TrackInfo and AlbumInfo for
copy.deepcopy (use also track_id and album_id) #1547
Apply_metadata tidied up
On the way to become generic. Used dictionnaries to map item attributes
to ItemInfo keys

def __reduce__(self):
"""Used by :py:func:`copy.deepcopy`"""
return self['album_id']

This comment has been minimized.

Copy link
@dosoe

dosoe Aug 14, 2017

Contributor

That's what I was looking for! How did you know this is the missing part for deepcopy?

This comment has been minimized.

Copy link
@gabrielhdt

gabrielhdt Aug 14, 2017

Author

I looked into copy.deepcopy ... you can see it on the pickle doc too:
https://docs.python.org/2/library/pickle.html#object.reduce

'artist_sort': 'artist_sort',
'artist_credit': 'artist_credit',
'mb_artistid': 'artist_id',
}

This comment has been minimized.

Copy link
@dosoe

dosoe Aug 14, 2017

Contributor

Why not just make a loop over all tags? Then when one adds a tag he doesn't need to add something here.

This comment has been minimized.

Copy link
@gabrielhdt

gabrielhdt Aug 14, 2017

Author

What you suggest must be eventually done. For the moment I adapted the current process to behave in a 'generic-friendly' way. I will come back to apply_metadata after having edited the track_info function of mb.py to make it capable of copying tags from recording object to TrackInfo.

@dosoe

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2017

Thanks! Then I will just kill my own try (that failed on deepcopy) here on #2650

New ItemInfo populating method
ItemInfo populated using a model (the alias dict) which is parsed to
automatically set attributes.
@gabrielhdt

This comment has been minimized.

Copy link
Author

commented Sep 2, 2017

It seems that perfect flexibility (i.e. dumping directly musicbrainz data to tags) isn't possible, due to the recursive architecture of musicbrainz relations (relations that have relations, which can be seen in musicbrainzngs dict as they are recursive) which can't be applied to the media tags.

Therefore, I chose to base the tagging process on the parsing of a mapping. The mapping reproduces the architecture of the musicbrainzngs dict and maps the musicbrainzngs dict key to the tag key (the ItemInfo attribute/key). Flexibility is improved as the mapping should eventually be stored in a json or yaml file, which will have to be updated to follow musicbrainz database schemes updates, one shouldn't need to edit python code to add an attribute, only update model.

Tags that need processing are treated separately (e.g. track index).

adapted unittests and adpated code for unittests
Old behaviour must be reproduced (to avoid failures in unit tests)
'arranger': 'arranger',
'media': 'media',
'disctitle': 'disctitle',
}

This comment has been minimized.

Copy link
@dosoe

dosoe Sep 2, 2017

Contributor

Hi! Thanks for all the work, it looks awesome. A question: will it be necessary for each new tag to add a corresponding line to the mapping?

This comment has been minimized.

Copy link
@gabrielhdt

gabrielhdt Sep 2, 2017

Author

Yes it will. But that shouldn't be a problem, since the only task will eventually be to add a line to a json/yaml file.

almose reproduced ancient behaviour, generic way
Special function have to be implemented, database management must be
improved (e.g. dynamically adding columns)
@dosoe

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2017

I'm wondering (I didn't try it out) how it works if there are several albumartists: what is currently done is that the names of all the artists are appended in a string, does something similar happen here or does he just take the last one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.