gboudreau / XBMCnfoMoviesImporter.bundle

688 stars 158 forks source link

Minor PEP 8 changes #76

Closed labrys closed 7 years ago

labrys commented 7 years ago

This PR implements numerous minor PEP8 fixes, restricted to only those that do not substantial alter the existing code. Further PEP8 changes will be the subject of a separate PR.

Issues addressed: line spacing, whitespace, comments, lambda assignment, truth comparison, imports

SlrG commented 7 years ago

Looks good. :) Thank you for contributing. Maybe you can have a look at the XBMCnfoTVImporter, too?

labrys commented 7 years ago

@SlrG Once I finish with movies I'll do the same for the TV importer... in fact I might play with combining them into a single plugin if possible (don't know if you've tried that in the past).

SlrG commented 7 years ago

You seem very well versed in python. More than myself. :) Thank you very much for your contributions! I have sadly very little time and there are some things that could be improved in the agents. (e.g. support of other file types than jpg) So if you have time, ideas and fun in working on the agents, feel free to continue improving them.

labrys commented 7 years ago

@SlrG Regarding the ID hash generated when an ID is missing. I'd like to change the way its generated. Obviously this will result in the ID hash being calculated differently. Do you know of any repercussions this might have?

I'd like to simplify it from the current:

# if movie id doesn't exist, create
# one based on hash of title and year
def ord3(x):
    return '%.3d' % ord(x)

id = int(''.join(map(ord3, media.name + str(media.year))))
id = str(abs(hash(int(id))))

to

id = hash((media.name, media.year))
id = 'hash {}'.format(id)  # prefix hash to prevent accidental collision with real ids
SlrG commented 7 years ago

I think this will be okay. On a sidenote, some user reported that it would/should be possible to have a fallback agent scan for results, if no id is returned, but I was never able to get it to work and the user never responded to my questions again. Check issue nr 65 for the movies agent and nr 72 for the tv agent. So it might be necessary in the future to remove the id generation bit. Maybe also take a look at plex' own agents on how and if they generate ids if not available. Maybe we get some hints there.