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

Make Tags more flexible to implement #2650

Open
wants to merge 25 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@dosoe
Copy link
Contributor

commented Aug 2, 2017

Hi!
This is a pull request that is supposed to implement what has been discussed on #1547 .

@mention-bot

This comment has been minimized.

Copy link

commented Aug 2, 2017

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

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

Is there a plan to also simplify all the beet/library.py stuff?

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

This first attempt where I only modified the TrackInfo function of beet/autotag/hooks.py seems to work, it installed itself without errors and beet update seems to work without complaints.

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

beet mbsync complains:

Traceback (most recent call last):
  File "/usr/bin/beet", line 11, in <module>
    load_entry_point('beets==1.4.6', 'console_scripts', 'beet')()
  File "/usr/lib/python3.6/site-packages/beets/ui/__init__.py", line 1256, in main
    _raw_main(args)
  File "/usr/lib/python3.6/site-packages/beets/ui/__init__.py", line 1243, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/usr/lib/python3.6/site-packages/beetsplug/mbsync.py", line 71, in func
    self.singletons(lib, query, move, pretend, write)
  File "/usr/lib/python3.6/site-packages/beetsplug/mbsync.py", line 86, in singletons
    track_info = hooks.track_for_mbid(item.mb_trackid)
  File "/usr/lib/python3.6/site-packages/beets/autotag/hooks.py", line 592, in track_for_mbid
    return mb.track_for_id(recording_id)
  File "/usr/lib/python3.6/site-packages/beets/autotag/mb.py", line 496, in track_for_id
    return track_info(res['recording'])
  File "/usr/lib/python3.6/site-packages/beets/autotag/mb.py", line 193, in track_info
    data_url=track_url(recording['id']),
  File "/usr/lib/python3.6/site-packages/beets/autotag/hooks.py", line 140, in __init__
    for k, v in kwargs.iteritems():
AttributeError: 'dict' object has no attribute 'iteritems'
@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

because I changed class TrackInfo(object): into class TrackInfo(dict):

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

It seems like we need to know which methods are used on TrackItem objects and either stop using them by changing the calls or implement them in the dict-like structure we want to use.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

Awesome! This is looking really good already—using __getattr__ to make things work more or less as they did before seems like a good direction. 🎉

Is there a plan to also simplify all the beet/library.py stuff?

Not currently… I actually rather like how things are in library. 😃 What did you have in mind as a candidate for simplification?

AttributeError: 'dict' object has no attribute 'iteritems'

For this, just use .items() instead of .iteritems(). (That works on Python 2 and Python 3; the latter is Python 2 only.)

Also, I'm not 100% convinced of the need for dictionaries in args—for example, TrackInfo({ 'foo': 'bar' }). For our purposes, just using TrackInfo(foo='bar') will work fine. If we ever need to construct TrackInfo objects from dictionaries, using TrackInfo(**d) will be convenient enough.

for arg in args:
if isinstance(arg, dict):
for k, v in arg.iteritems():
self[k] = v

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 2, 2017

Member

Here, FWIW, is where I'm proposing we probably don't need args.

This comment has been minimized.

Copy link
@dosoe

dosoe Aug 2, 2017

Author Contributor

I removed the *args part. It's not like I know what I'm doing, just copy-pasting from https://stackoverflow.com/questions/2352181/how-to-use-a-dot-to-access-members-of-dictionary/32107024#32107024

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

Well, right now when we want to add a new tag we still have to implement it into library.py to set the type of the tag. Couldn't he just recognize it automatically?

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

I guess the final goal is to just need to make a loop on all keys of TrackInfo to write them to tags instead of putting them one by one manyally as it is done right now.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Aug 2, 2017

That actually works already! You don't need to list tags in library.py; you can just do item.foo = bar for any foo and it will just work as a "flexible attribute." Listing things in library.py is just for a smaller set of "fixed attributes." Fixed attributes gain additional benefits (efficiency in storage & querying; a type declaration), but it's not necessary to get basic functionality.

More detail in this blog post: http://beets.io/blog/flexattr.html

@@ -45,13 +45,13 @@ def apply_item_metadata(item, track_info):
if track_info.data_source:
item.data_source = track_info.data_source

if track_info.lyricist is not None:
if track_info.get('lyricist'):

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 2, 2017

Member

To minimize changes like this, we could have __getattr__ return None by default for missing fields. Not very Pythonic, but could be fine for these classes…

This comment has been minimized.

Copy link
@dosoe

dosoe Aug 2, 2017

Author Contributor

If I'm reading right, it is already the case:

def __getattr__(self, attr):
        return self.get(attr)

This comment has been minimized.

Copy link
@sampsyo

sampsyo Aug 2, 2017

Member

Good point; sorry I didn't notice that. (Perhaps it would be useful to add a doctoring to that effect?) So maybe we can leave these checks how they are, just to keep a clean diff.

This comment has been minimized.

Copy link
@dosoe

dosoe Aug 2, 2017

Author Contributor

Yes, that is what I wanted to do.

This comment has been minimized.

Copy link
@dosoe

dosoe Aug 2, 2017

Author Contributor

What is a 'doctoring'?

This comment has been minimized.

Copy link
@dosoe

dosoe Aug 2, 2017

Author Contributor

to be perfectly honest, I find these tests if track_info.XX is not None: extremely ugly, that's why I removed them.

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

This now seems to work with mbsync as well now.

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

Some of the tests are not happy, could that be because the declaration of item changed? See the changes in mb.py, are there similar calls in the tests?

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

Now I wonder about __init__.py: Should I just make a loop through all keys of the track_info object and add them all at once (maybe with a few exceptions for specific cases) or should I keep it like this?

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

I would like to get it working with TrackInfo before doing the same with AlbumInfo.

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

autotag/match.py seems to have a hard time related to TypeError: unhashable type: 'TrackInfo' when doing extra_tracks = list(set(tracks) - set(mapping.values())) at line 114.

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

The other error I get is TypeError: 'NoneType' object is not callable in test/test_autotag.py when I do copy.deepcopy(self.info) at various places. After learning what a deepcopy is, it turns out that it uses hashtables, which are not applicable to dicts, so when he tries to deepcopy a list of dicts, he runs into problems because of that (or that's how I understand it). All the isues seem to revolve around the fact that a python dict is not hashable.

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

I also get an error in the distance class in beets/autotag/match.py, also a TypeError: unhashable type: 'TrackInfo'

dosoe added some commits Aug 6, 2017

dosoe added some commits Aug 6, 2017

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2017

Making a __hash__ function solves some of the errors, however it is still not possible to deepcopy a list of TrackInfo objects.

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2017

from https://bugs.python.org/issue16251 it seems like deepcopy doesn't work because in some cases the __getattr__ function returns None . However we would like it to stay like that.

dosoe added some commits Aug 7, 2017

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2017

It looks like someone did make it work here: #2654 , so I will give this PR up if the other one gets accepted.

dosoe added some commits Dec 14, 2017

@dosoe

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2017

It now seems to boil down to differences of deepcopy between python2 and python3.

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.