When "album" tag not set and "artist" tag is set, a picture of the ar…

[rocus]

When "album" tag not set and "artist" tag is set, a picture of the artist is loaded by "covers_lastfm".
Better a picture of the artist than no picture!!

[multani]

All of this could be greatly simplified:

• define your URL endpoint once
• define a base dictionary to hold the parameters
• update the dictionary base on not album (no need for the extra variable, nor for the double check)
• produce a final search_url based on the previous definitions

Please respect PEP 8 formatting for this, also (related to spaces and size of the lines).

[rocus]

On 21.06.2015 20:48, Jonathan Ballet wrote:

In sonata/plugins/covers_lastfm.py
#86 (comment):

@@ -35,14 +35,17 @@ def on_cover_fetch(artist, album, on_save_cb, on_err_cb):
opener.addheaders = [("User-Agent", make_user_agent())]

 # First, find the link to the master release of this album

• search_url = "http://ws.audioscrobbler.com/2.0/?%s" % (

•    urllib.parse.urlencode({
  

•        "method": "album.getInfo",
  

•        "artist": artist,
  

•        "album": album,
  

•        "api_key": API_KEY,
  

•        "format": "json",
  

•    }))
  

• album_not_given = album == "" or album == None
• if album_not_given:

•    search_url = "http://ws.audioscrobbler.com/2.0/?%s" % ( urllib.parse.urlencode({ "method": "artist.getInfo", "artist":
  

All of this could be greatly simplified:

• define your URL endpoint once
• define a base dictionary to hold the parameters
• update the dictionary base on |not album| (no need for the extra
  variable, nor for the double check)
• produce a final |search_url| based on the previous definitions

Please respect PEP 8 formatting
https://www.python.org/dev/peps/pep-0008/ for this, also (related to
spaces and size of the lines).

—
Reply to this email directly or view it on GitHub
https://github.com/multani/sonata/pull/86/files#r32896102.

I understand your remark but I coud not write this the way you suggest
(due to my limited knowledge of Python).
you mean that "not album" is the same as "album="" and album=None". I
was not sure about that.

Spaces: you mean there are tabs? I use vi (a bit oldfashoned).

You understand that it sometimes is easier to duplicate code and change
one branch a bit that not duplicating (although that is better
ofcourse)? I am sorry: a beginning python programmer talking.

[multani]

I understand your remark but I coud not write this the way you suggest (due to my limited knowledge of Python). you mean that "not album" is the same as "album="" and album=None". I was not sure about that.

not album, in this case, would be equal to album="" or album=None, as the documention says. In some cases, I do not advice in using this form, but here would be OK.

Spaces: you mean there are tabs? I use vi (a bit oldfashoned).

I meant this : https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements
Please write (urllib.parse.urlencode({"method": instead of ( urllib.parse.urlencode({ "method":, and so on. Also, limit your lines width to 80 characters.

You understand that it sometimes is easier to duplicate code and change one branch a bit that not duplicating (although that is better ofcourse)? I am sorry: a beginning python programmer talking.

I would be interested to know a good example of where duplicated code is a good thing.

You can test your work by duplicating if it easier for you, but in this case, it will be much better not to duplicate, especially since I really had to look carefully to see what was the difference between the 2 code paths, although there's only one variable which is different.

[rocus]

Hi Jonathan,

I have made some changes:

removed duplicate code in the if statement.
removed the album_not_given variable and used "not album"

B.T.W. I said than duplicate code in some cases makes things easier, I
did not say it made things better.

I still have doubts about the last_fm covers plugin because it only
delivers one picture (and not the six to choose from like in the old
rhapsody program). I also think that if no picture is found (for example
because of a wrong album tag) it is still better to show one picture of
the artist.

I saw one remark of you that the plugin tries very hard to find a
picture. Was this remark about another plugin that you are writing
(because last_fm_covers tries to find pictures just at one place)

I committed my changes to my artist-jpg branch and pushed it to my
github artist-jpg branch. Should I do more (a pull request? or something
else?)

On 06/22/2015 10:49 PM, Jonathan Ballet wrote:

In sonata/plugins/covers_lastfm.py
#86 (comment):

@@ -35,14 +35,17 @@ def on_cover_fetch(artist, album, on_save_cb, on_err_cb):
opener.addheaders = [("User-Agent", make_user_agent())]

 # First, find the link to the master release of this album

• search_url = "http://ws.audioscrobbler.com/2.0/?%s" % (

•    urllib.parse.urlencode({
  

•        "method": "album.getInfo",
  

•        "artist": artist,
  

•        "album": album,
  

•        "api_key": API_KEY,
  

•        "format": "json",
  

•    }))
  

• album_not_given = album == "" or album == None
• if album_not_given:

•    search_url = "http://ws.audioscrobbler.com/2.0/?%s" % ( urllib.parse.urlencode({ "method": "artist.getInfo", "artist":
  

I understand your remark but I coud not write this the way you
suggest (due to my limited knowledge of Python). you mean that
"not album" is the same as "album="" and album=None". I was not
sure about that.

|not album|, in this case, would be equal to |album="" or album=None|,
as the documention says
https://docs.python.org/3/library/stdtypes.html#truth-value-testing.
In some cases, I do not advice in using this form, but here would be OK.

Spaces: you mean there are tabs? I use vi (a bit oldfashoned).

I meant this :
https://www.python.org/dev/peps/pep-0008/#whitespace-in-expressions-and-statements
Please write |(urllib.parse.urlencode({"method":| instead of |(
urllib.parse.urlencode({ "method":|, and so on. Also, limit your lines
width to 80 characters.

You understand that it sometimes is easier to duplicate code and
change one branch a bit that not duplicating (although that is
better ofcourse)? I am sorry: a beginning python programmer talking.

I would be interested to know a good example of where duplicated code
/is/ a good thing.

You can test your work by duplicating if it easier for you, but in
this case, it will be much better not to duplicate, especially since
I really had to look carefully to see what was the difference between
the 2 code paths, although there's only one variable which is different.

—
Reply to this email directly or view it on GitHub
https://github.com/multani/sonata/pull/86/files#r32981589.

[multani]

Please have look at the comments I made.

Also, can you give me an artist name which lacks an album image but has an artist image, so I can test your patch? Thanks!

[rocus]

On 21.06.2015 20:49, Jonathan Ballet wrote:

Please have look at the comments I made.

Also, can you give me an artist name which lacks an album image but
has an artist image, so I can test your patch? Thanks!

—
Reply to this email directly or view it on GitHub
#86 (comment).

You can easily test that if you take an arbitrary mp3 file. First you
check that it has an artist and an album tag. Then it should show the
album cover. Now if you edit the embedded tags and leave the album tag
(empty or non existent) then it should show you an image of the artist.

I have a lot of mp3's downloaded (with mlmule) and many of them have no
album tag. These mp3's show now at least an artist image.

So it is all a matter of how accurate your tags are. If you buy
everything (as you should, I suppose) then the tags are problably
correct, otherwise many people hardly care....

[multani]

On 06/22/2015 05:49 PM, Rocus wrote:

You can easily test that if you take an arbitrary mp3 file. First you
check that it has an artist and an album tag. Then it should show the
album cover. Now if you edit the embedded tags and leave the album tag
(empty or non existent) then it should show you an image of the artist.

Thanks, I will have a look.