Fix pylint and flake8 issues #194

dagwieers · 2019-09-08T20:11:28Z

Check if this PR fulfills these requirements:

My changes respect the Kodi add-on development rules
I have read the CONTRIBUTING document.
I made sure there wasn't another one Pull Requests opened for the same update/change
I have successfully tested my changes locally

Types of changes

New feature (non-breaking change which adds functionality)
Feature change (non-breaking change which change behaviour of an existing functionality)
Improvement (non-breaking change which improve functionality)
Bug fix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to change)

Description

This is a set of fixes for known pylint and flake8 issues.
The goal is to enable pylint and tox by default so new code
is properly tested and conforms to pylint and flake8 checks.

resources/lib/database/db_shared.py

dagwieers · 2019-09-08T22:10:22Z

Ready for review. Details at: https://travis-ci.org/dagwieers/plugin.video.netflix/builds/582479348
@CastagnaIT Can you enable Travis support for this repository?

dagwieers · 2019-09-08T22:50:00Z

All pylint and flake8 issues are now fixed in this PR.

Smeulf · 2019-09-08T23:54:27Z

I hope changing iteritems to itens won't slow down the add-on, as it's already pretty CPU consuming on a Raspberry Pi.

dagwieers · 2019-09-08T23:57:26Z

@Smeulf I agree, we need to test the performance impact. Once we have integrations tests implemented, I would like to look at performance and see where we can improve the add-on.

One of the slowdowns is the use of Requests on Raspberry Pi, for VRT NU and InputStream Helper we moved to using urllib/urllib2. Another slowdown is the imports at the top of files. In some cases it is better to import modules when they are needed (it depends of code paths, are they avoidable or not).

dagwieers · 2019-09-08T23:59:49Z

I also don't mind splitting off the Python 3 stuff from this PR. But unfortunately it does not make sense to do that on the master branch because it would lead to conflicts later.

Smeulf · 2019-09-08T23:59:55Z

Be aware I have both Pi2 and Pi3, so I can run tests under LibreELEC and OSMC on both versions, should you give me the proper tools ;)

No Pi4 (Yet^^).

dagwieers · 2019-09-09T00:10:56Z

@Smeulf The slow urllib3 (requests) imports have been fixed in the meantime. But it used to take 4 seconds in later releases. Now it is 100ms on import, but possible slow on first requests.

Smeulf · 2019-09-09T00:40:12Z

Are you sure it's a really good idea to merge it now in master?
Shouldn't a specific branch be created to check perfoemances issues?

I'm pertty confident about what you're doing, but... Well you know ;)

dagwieers · 2019-09-09T00:42:42Z

@Smeulf In my tests on Raspberry Pi 3 I don't see any real differences though. I think any performance wins are more likely to be found elsewhere than this.

Smeulf · 2019-09-09T00:45:01Z

on Raspberry Pi 3 I don't see any real differences

Didn't know you have one :) so be it!

dagwieers · 2019-09-09T00:47:33Z

@Smeulf I have RPi2, RPi3 and RPi4 in use today. But RPi2 and RPi3 are used most of the time ;-)

PS add-ons/plugin.video.vrt.nu#169

dagwieers · 2019-09-09T01:54:34Z

I got my integration tests working:

Debug: [plugin.video.netflix (0)] Cache entry show_80200596 in cache_artinfo has expired => cache miss
Debug: [plugin.video.netflix (0)] Cache entry show_80135414 in cache_infolabels has expired => cache miss
Debug: [plugin.video.netflix (0)] Cache entry show_80135414 in cache_artinfo has expired => cache miss
-=( My List )=-
» Stranger Things → plugin://plugin.video.netflix/directory/show/80057281/
» Friends → plugin://plugin.video.netflix/directory/show/70153404/
» Peaky Blinders → plugin://plugin.video.netflix/directory/show/80002479/
» Fargo → plugin://plugin.video.netflix/directory/show/70285785/
» Narcos → plugin://plugin.video.netflix/directory/show/80025172/
» Homeland → plugin://plugin.video.netflix/directory/show/70180387/
» Suits → plugin://plugin.video.netflix/directory/show/70195800/
» The IT Crowd → plugin://plugin.video.netflix/directory/show/70140450/
» Ozark → plugin://plugin.video.netflix/directory/show/80117552/
» Conversations with a Killer: The Ted Bundy Tapes → plugin://plugin.video.netflix/directory/show/80226612/
» Rick and Morty → plugin://plugin.video.netflix/directory/show/80014749/
· The Highwaymen → plugin://plugin.video.netflix/play/movie/80200571/
» Better Call Saul → plugin://plugin.video.netflix/directory/show/80021955/
» Bron → plugin://plugin.video.netflix/directory/show/70254188/
» The Affair → plugin://plugin.video.netflix/directory/show/80027745/
» Las chicas del cable → plugin://plugin.video.netflix/directory/show/80100929/
» The Haunting of Hill House → plugin://plugin.video.netflix/directory/show/80189221/
» Black Mirror → plugin://plugin.video.netflix/directory/show/70264888/
» Happy! → plugin://plugin.video.netflix/directory/show/80177346/
» Gravity Falls → plugin://plugin.video.netflix/directory/show/80017382/
» GLOW → plugin://plugin.video.netflix/directory/show/80114988/
· The Great Hack → plugin://plugin.video.netflix/play/movie/80117542/
» Eigen kweek → plugin://plugin.video.netflix/directory/show/80015504/
» World War II in Colour → plugin://plugin.video.netflix/directory/show/70254851/
» Sherlock → plugin://plugin.video.netflix/directory/show/70202589/
» The End of the F***ing World → plugin://plugin.video.netflix/directory/show/80175722/
» Gomorra - de serie → plugin://plugin.video.netflix/directory/show/80079255/
» The Umbrella Academy → plugin://plugin.video.netflix/directory/show/80186863/
· Gone Girl → plugin://plugin.video.netflix/play/movie/70305893/
» Disenchantment → plugin://plugin.video.netflix/directory/show/80095697/
» The Disappearance of Madeleine McCann → plugin://plugin.video.netflix/directory/show/80194956/
» You → plugin://plugin.video.netflix/directory/show/80211991/
» The Fall → plugin://plugin.video.netflix/directory/show/70272726/
» De Ridder → plugin://plugin.video.netflix/directory/show/81033290/
» Arrested Development → plugin://plugin.video.netflix/directory/show/70140358/
· The Dark Knight → plugin://plugin.video.netflix/play/movie/70079583/
» Santa Clarita Diet → plugin://plugin.video.netflix/directory/show/80095815/
» Grace and Frankie → plugin://plugin.video.netflix/directory/show/80017537/
» Atypical → plugin://plugin.video.netflix/directory/show/80117540/
» Shooter → plugin://plugin.video.netflix/directory/show/80109194/
· Blade Runner: The Final Cut → plugin://plugin.video.netflix/play/movie/70082907/
· Er ist wieder da → plugin://plugin.video.netflix/play/movie/80094357/
· Django Unchained → plugin://plugin.video.netflix/play/movie/70230640/
» Tytgat Chocolat → plugin://plugin.video.netflix/directory/show/80229824/
» Orphan Black → plugin://plugin.video.netflix/directory/show/70276033/
· Bird Box → plugin://plugin.video.netflix/play/movie/80196789/
» Parfum → plugin://plugin.video.netflix/directory/show/80200596/
» The Mist → plugin://plugin.video.netflix/directory/show/80135414/

But I need this PR to be merged first before I can start adding the tests.
We also need Travis enabled so that we can add credentials in Travis for these integration tests.

dagwieers · 2019-09-09T02:13:18Z

My integration tests are failing on Travis and I guess I need @CastagnaIT or @Smeulf to look at what is going on. https://travis-ci.org/dagwieers/plugin.video.netflix/builds/582479467

Running service.py works fine locally, but fails on Travis currently.

Smeulf · 2019-09-09T03:06:12Z

The only variable used to perform the handshake is the esn.
So as you use a custom one, you should try without, and let the addon generate the esn.

Not sure it will work :/

dagwieers · 2019-09-09T12:31:54Z

So you mean I should not set both NETFLIX_USERNAME and NETFLIX_PASSWORD, and also provide a NETFLIX_ESN ?

Smeulf · 2019-09-09T12:46:05Z

Yes, the username and password should be sufficient.

Setting a custom esn is used mostly for Android when 4K is not available.

If no esn is set (most of the time) the add-on should generate one (based on the profile iirc).

dagwieers · 2019-09-09T12:48:59Z

Very good. That will increase coverage ;-)

PS While testing the integration tests (on my Intel laptop) things are very slow, and I can see things running repeatedly that shouldn't be run repeatedly IMO. (This is unrelated to any recent changes, because the same code works fine on Netflix, it might be that e.g. debug-mode is just a lot slower).

dagwieers · 2019-09-09T12:53:28Z

I hope changing iteritems to itens won't slow down the add-on, as it's already pretty CPU consuming on a Raspberry Pi.

I think there are some better solutions for iteritems than the one proposed by kodi-addon-checker. https://python-future.org/compatible_idioms.html#lists-versus-iterators

Smeulf · 2019-09-09T12:54:55Z

That's exactly the article I had in mind when asking that question! :D

Smeulf · 2019-09-09T12:56:50Z

I can see things running repeatedly that shouldn't be run repeatedly IMO.

Any example?

dagwieers · 2019-09-09T12:59:10Z

@Smeulf The profiles are being processed repeatedly (very slowly) every time (and sometimes more than once). Have you run these integration tests make run on your own system for testing?

Smeulf · 2019-09-09T13:48:51Z

I didn't run make run but I can see the behaviour in live with Kodi debug enabled.

Everytime the addon.py is launch, we send a web request to get the profile list. That seems pretty good as we need to ensure the profile still exists.

But that request takes between 200 and 800ms (here on Windows, I assume on Pi it's worse).

The second profile request is made before request to some lists or data. As if we validate again the profile still exists. And there I don't know why.

This is a set of fixes for known pylint and flake8 issues. The goal is to enable pylint and tox by default so new code is properly tested and conforms to pylint and flake8 checks.

dagwieers · 2019-09-09T15:21:31Z

@Smeulf I removed the Python 3 stuff from this PR. Thanks for your insights ;-)

dagwieers · 2019-09-09T15:22:25Z

Please merge! And enable Travis support.

https://travis-ci.org/dagwieers/plugin.video.netflix/builds/582739408

Smeulf reviewed Sep 8, 2019

View reviewed changes

resources/lib/database/db_shared.py Show resolved Hide resolved

dagwieers marked this pull request as ready for review September 8, 2019 22:09

dagwieers changed the title ~~Fix pylint issues~~ Fix pylint and flake8 issues Sep 8, 2019

dagwieers mentioned this pull request Sep 8, 2019

Add the Netflix add-on to the official Kodi repository #191

Closed

2 tasks

dagwieers mentioned this pull request Sep 9, 2019

Integration tests #196

Merged

9 tasks

dagwieers added 3 commits September 9, 2019 17:20

Fix pylint and flake8 issues

bf10972

This is a set of fixes for known pylint and flake8 issues. The goal is to enable pylint and tox by default so new code is properly tested and conforms to pylint and flake8 checks.

Separate run tests in Makefile

e1a59ce

Fixes

097de1e

CastagnaIT merged commit 2dcf225 into CastagnaIT:master Sep 9, 2019

This was referenced Sep 18, 2019

Small fixes after pylint/flake cleanup #218

Merged

Version bump (0.15.3) #212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pylint and flake8 issues #194

Fix pylint and flake8 issues #194

dagwieers commented Sep 8, 2019 •

edited

Loading

dagwieers commented Sep 8, 2019 •

edited

Loading

dagwieers commented Sep 8, 2019 •

edited

Loading

Smeulf commented Sep 8, 2019

dagwieers commented Sep 8, 2019

dagwieers commented Sep 8, 2019

Smeulf commented Sep 8, 2019

dagwieers commented Sep 9, 2019 •

edited

Loading

Smeulf commented Sep 9, 2019

dagwieers commented Sep 9, 2019

Smeulf commented Sep 9, 2019

dagwieers commented Sep 9, 2019

dagwieers commented Sep 9, 2019

dagwieers commented Sep 9, 2019

Smeulf commented Sep 9, 2019 •

edited

Loading

dagwieers commented Sep 9, 2019

Smeulf commented Sep 9, 2019

dagwieers commented Sep 9, 2019 •

edited

Loading

dagwieers commented Sep 9, 2019

Smeulf commented Sep 9, 2019

Smeulf commented Sep 9, 2019

dagwieers commented Sep 9, 2019

Smeulf commented Sep 9, 2019 •

edited

Loading

dagwieers commented Sep 9, 2019

dagwieers commented Sep 9, 2019 •

edited

Loading

Fix pylint and flake8 issues #194

Fix pylint and flake8 issues #194

Conversation

dagwieers commented Sep 8, 2019 • edited Loading

Check if this PR fulfills these requirements:

Types of changes

Description

dagwieers commented Sep 8, 2019 • edited Loading

dagwieers commented Sep 8, 2019 • edited Loading

Smeulf commented Sep 8, 2019

dagwieers commented Sep 8, 2019

dagwieers commented Sep 8, 2019

Smeulf commented Sep 8, 2019

dagwieers commented Sep 9, 2019 • edited Loading

Smeulf commented Sep 9, 2019

dagwieers commented Sep 9, 2019

Smeulf commented Sep 9, 2019

dagwieers commented Sep 9, 2019

dagwieers commented Sep 9, 2019

dagwieers commented Sep 9, 2019

Smeulf commented Sep 9, 2019 • edited Loading

dagwieers commented Sep 9, 2019

Smeulf commented Sep 9, 2019

dagwieers commented Sep 9, 2019 • edited Loading

dagwieers commented Sep 9, 2019

Smeulf commented Sep 9, 2019

Smeulf commented Sep 9, 2019

dagwieers commented Sep 9, 2019

Smeulf commented Sep 9, 2019 • edited Loading

dagwieers commented Sep 9, 2019

dagwieers commented Sep 9, 2019 • edited Loading

dagwieers commented Sep 8, 2019 •

edited

Loading

dagwieers commented Sep 8, 2019 •

edited

Loading

dagwieers commented Sep 8, 2019 •

edited

Loading

dagwieers commented Sep 9, 2019 •

edited

Loading

Smeulf commented Sep 9, 2019 •

edited

Loading

dagwieers commented Sep 9, 2019 •

edited

Loading

Smeulf commented Sep 9, 2019 •

edited

Loading

dagwieers commented Sep 9, 2019 •

edited

Loading