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

Didl class for Sonos favorites #478

Merged
merged 9 commits into from Jan 6, 2018

Conversation

Projects
None yet
3 participants
@ghcs27
Copy link
Member

commented Feb 22, 2017

This PR makes browsing Sonos favorites with the existing music library methods possible.
This is archieved by adding a data structure for the object.itemobject.item.sonos_favorite Didl class. The metadata of the favorites contains a <resMD> tag, which encloses the metadata of the item to which the favorite refers. I have added a reference property to make this metadata accessible as a Didl object.
Unlike the existing implementation, one can now handle Sonos and radio favorites just like other music items - the specific methods are no longer required.
I would really like to integrate this functionality into SoCo, so please let me know if I should change something to get this merged!

@ghcs27 ghcs27 referenced this pull request Feb 22, 2017

Closed

Get sonos favourites #402

@coveralls

This comment has been minimized.

Copy link

commented Feb 22, 2017

Coverage Status

Coverage decreased (-0.02%) to 61.018% when pulling 8952036 on ghcs27:favorite_data_structure_alt into 9de0596 on SoCo:master.

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from c155cea to 6ad1420 Feb 22, 2017

@ghcs27

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2017

Can someone please help me with this complaint:
soco/data_structures.py:772:15: F821 undefined name 'from_didl_string'
The code seems to work fine, so I don't understand what this is about.

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Hi @ghcs27

This sounds real interesting and I will review this next.

@ghcs27 ghcs27 referenced this pull request Mar 22, 2017

Open

Release 1.0 #316

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from 6ad1420 to 2e4e9f9 Apr 14, 2017

@coveralls

This comment has been minimized.

Copy link

commented Apr 14, 2017

Coverage Status

Coverage increased (+0.4%) to 60.72% when pulling 2e4e9f9 on ghcs27:favorite_data_structure_alt into 5ceb384 on SoCo:master.

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from 2e4e9f9 to 0aa326e May 1, 2017

use reference property instead of attribute initialized in __init__ s…
…o that changing the reference object will update resMD

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from 0aa326e to 98946f9 May 1, 2017

@coveralls

This comment has been minimized.

Copy link

commented May 1, 2017

Coverage Status

Coverage decreased (-0.04%) to 60.723% when pulling 98946f9 on ghcs27:favorite_data_structure_alt into 06a5e75 on SoCo:master.

@ghcs27

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

I just figured out where soco/data_structures.py:772:15: F821 undefined name 'from_didl_string' comes from: from_didl_string() was moved to data_structures_entry.py.
However, I cannot just do

from .data_structures_entry import from_didl_string

because that would create a cyclic import. Could someone please help me to resolve this?

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from e4d710d to 76b4909 Jun 14, 2017

@coveralls

This comment has been minimized.

Copy link

commented Jun 14, 2017

Coverage Status

Coverage decreased (-56.5%) to 4.294% when pulling e4d710d on ghcs27:favorite_data_structure_alt into 06a5e75 on SoCo:master.

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from 76b4909 to c59a09e Jun 15, 2017

@coveralls

This comment has been minimized.

Copy link

commented Jun 15, 2017

Coverage Status

Coverage decreased (-0.06%) to 60.702% when pulling c59a09e on ghcs27:favorite_data_structure_alt into 06a5e75 on SoCo:master.

@SoCo SoCo deleted a comment from coveralls Jun 18, 2017

@SoCo SoCo deleted a comment from coveralls Jun 18, 2017

@SoCo SoCo deleted a comment from coveralls Jun 18, 2017

@SoCo SoCo deleted a comment from coveralls Jun 18, 2017

@SoCo SoCo deleted a comment from coveralls Jun 18, 2017

@SoCo SoCo deleted a comment from coveralls Jun 18, 2017

@SoCo SoCo deleted a comment from coveralls Jun 18, 2017

@ghcs27

This comment has been minimized.

Copy link
Member Author

commented Dec 1, 2017

Just tried import .data_structures_entry instead of from .data_structures_entry import from_didl_string,
but pylint still complains about the cyclic import. A solution that feels a bit more like a hack would be injecting a method from one module into the other:

# module_1

functions = []

code
...
for fun in functions:
    fun(x)
...
code
# module_2

import module_1

def foo(bar):
    ...

module_2.functions.append(foo)

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from 92ebeae to 8848c5b Dec 4, 2017

@ghcs27

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2017

Got the import fixed, but it still requires turning off cyclic import checking (globally!).
Please also have a look at this refactoring which tries to make the import structure more elegant, by using the trick described above (turns out it is actually a valid code pattern, if applied carefully).

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Dec 4, 2017

I must admit I don't care much for injecting objects into modules, as I don't imagine it will much good for readability. I have looked a little around and I think the pylint error is a bug, or, the circular relative imports aren't supported in earlier python 3 versions. Could you try and disable the warning globally, so we can see whether the tests run on all python version?

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from d7ad90b to 475768e Dec 5, 2017

@ghcs27

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2017

Hmm... it's a python error if I do from . import data_structures_entry.

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from 475768e to 099b44c Dec 7, 2017

@ghcs27

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2017

Neither does import soco.data_structures_entry as data_structures_entry.

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from 099b44c to ec0164b Dec 7, 2017

@ghcs27

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2017

Nor just import soco.data_structures_entry.
It might be that we can only resolve the cyclic import problem by changing all imports along the cycle
(or refactor so we don't need to).

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

As discussed on IRC. For now, I suggest you formalize you original implementation, so that the import is saved in a module level variable, which is initially set to None, then in the code that use it, check whether the function is imported and if not import it and save it to the global variable. It is the less intrusive way to get this merged soon.

If this can be done without triggering the cyclic import pylint warning, then disabling that warning should be removed, if not, then it should be left in and a separate issue should be opened to fix the code better.

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from ec0164b to c6d645e Dec 19, 2017

@ghcs27

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2017

Sorry, I forgot.

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

No problem.

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from c6d645e to 24eacd7 Dec 19, 2017

@ghcs27

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2017

Apparently pylint would like uppercase naming for the module level variable, and doesn't like the global statement:

C: 44, 0: Constant name "_from_didl_string" doesn't conform to UPPER_CASE naming style (invalid-name)
W:822, 8: Using the global statement (global-statement)
C:822, 8: Constant name "_from_didl_string" doesn't conform to UPPER_CASE naming style (invalid-name)

Should I rename it? Is there an alternative to global? Or should I just disable the warnings?

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Dec 19, 2017

Yes, rename to _FROM_DIDL_STRING_FUNCTION and disable the global warning on the line where you use it

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch 2 times, most recently from d106e48 to e71fd01 Dec 19, 2017

@ghcs27 ghcs27 force-pushed the ghcs27:favorite_data_structure_alt branch from e71fd01 to 4fc07fa Dec 19, 2017

@ghcs27

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2017

OK, now it's done (I hope).
pylint only complains about files I didn't touch.

@KennethNielsen

This comment has been minimized.

Copy link
Member

commented Jan 6, 2018

It seems the failing builds are due to a new pylint version on TravisCI. I'll merge this now and look into the pylint warnings afterwards. Thanks for the contributions. Remember to write and entry in the release notes issue #551.

@KennethNielsen KennethNielsen merged commit c500e34 into SoCo:master Jan 6, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@ghcs27 ghcs27 referenced this pull request Jan 6, 2018

Closed

Release notes 0.14 #551

@ghcs27 ghcs27 deleted the ghcs27:favorite_data_structure_alt branch Jan 11, 2018

@ghcs27 ghcs27 referenced this pull request Jan 14, 2018

Closed

Radio redux #328

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.