Added new function to set player name

[KennethNielsen]

Hi @scottgwaters

Awesome with yet another bit of functionality. I have a few suggestions, that are all more or less cosmetic.

You assign False as the default value. While I realize that something similar is done in other places in SoCo, I would suggest that we work towards avoiding it. I think it makes for better style if None (which has the special NoneType) is used as the default value in cases where you can choose whether you want to give an argument or not (like the volume method), and in all other cases the default value should be of the same type as the expected input i.e. False should only be used as default value if we are expecting a Boolean input. However, since this particular method does nothing if it has no input, I would actually suggest to not have a default value, so as to make the playername a mandatory argument for the method.

And then in the purely cosmetic category. Coding conventions suggests that words in variable names should be separated by an underscore, so I would suggest to use _playername (same way as you did in the method name) in all places where you now use playername. As the last thing, commas separating arguments in method calls or method definitions should be followed by a comma, applies both to line 93 and 107. I hope you will forgive the nitpicking, but Rahim mentioned that pep8 should be a goal, so we might as well get started.

BTW. If you do decide to change anything. I think you can just add any additional commits to this thread so as to keep it gathered.

Regards and keep up the amazing work. Kenneth

[scottgwaters]

Thanks for the feedback. I'll make those changes. New to python and coding in general. Not sure how to make commits to this thread. Also not sure what you mean by the commas can you give an example in the code that has them?

[KennethNielsen]

About the commits, what I was proposing was that you just post the commit hash here if you make any changes, so that @rahims can see that there are more commits. Actually, if you are interested, I think the easier way to develop features for a project is to use named branches. I still relatively new to the concept, but it seems really clever. So you will essentially create a branch for your new feature or bugfix and then make a pull request for that branch. That way you can continue to make commits to the branch based on feedback, without having to change what needs to be pulled. It also seems like a good way to be able to work with several different fixes/additions at a time. You can read more about it here: https://help.github.com/articles/fork-a-repo under "create branches" there's a "how do I use branches" button.

About the commas. The coding styles suggest: def function(arga, argb, argc=None, argd=0) instead of def function(arga,argb,argc=None,argd=0) to raise readability (and the same way when you call a function).

There is a whole list of these suggested style rules in the python style guide known as pep8: http://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements However, I believe that most people probably uses style checkers to not have to learn all of them by heart, that is however not so easy right now as the rest of the module does not follow the rules either. It is planned, but in the mean time we can just catch a few things in review and then tighten it up later.

[scottgwaters]

thanks now I understand about functions I'll make the fix. I'll look into the branch idea. The way it is now certainly is not ideal. I have pandora music service all figured out. I should be able to implement getting list of pandora stations and playing them to the sonos. I'm wondering if this should be separate class? I'm going to implement a get_music_service function that will get usernames of the music services subscribed to. But the entire pandora code I think might be better in it's own class?

[KennethNielsen]

That sounds great about the pandora music service. I hope you will share a few tips later about how you figured out the upnp commands, as I'm very interested in adding a music service on my own.

Regarding how to structure it I also think these music services should each have their own class. Besides that I was pondering a few different options for how to expose the music service methods in soco: 1) One idea would be to let soco inherit from all the music service classes and then adding a prefix to all the methods names in that music service class e.g. pandora_get_list_of_stations() and so on 2) Is essentially your idea. Implement each music service in its own class (and I think they should also each have their own file in a subdir). And then from soco implement a method to set the music services (which will instantiate the music service to a fixed variable within soco e.g. self.music_service) and one to get the music service instance.

I think by far that number 2 i the better option. It will however need a little importing magic in soco. I would suggest something like the: 1) Make a subdir in the soco archive named "music_services" 2) In that subdir implement a service as a class called "Pandora" in a file named pandora.py 3) In soco add the music service sub dir to the path in the init method: import os # Should off course be imported at the top of the file import sys # Should off course be imported at the top of the file socodir = os.path.abspath(file) sys.path.insert(0, socodir + os.sep + 'music_services') 4) In the set_music_service method that takes the capitalized name "Pandora" as an argument that is stored in the variable music_service_name, import and instantiate the service like so: music_service_module = import(music_service_name.lower()) music_service_class = getattr(music_service_module, music_service_name) self.music_service = music_service_class()

It may seem a little convoluted, but the issue is that python really isn't that friendly when it comes to working with modules and class based in a name you have in a variable.

What do you think?