Assorted fixes #9

dagwieers · 2019-12-15T01:45:39Z

This PR includes:

Fix a bug caused by the dot in the return signal name
Replace RECEIVER global with static variable
Replace getAddonInfo() call with _addon_id() using static variable
Fix SPDX to LGPL-2.1-or-later
~~Switch to using unicode_literals for Python 2~~
Add _to_unicode() function
Remove unused CallHandler.data
Use selective imports

And also cosmetic/testing improvements:

Add docstrings to all methods and functions
Remove disabled import-error and missing-docstring pylint tests
A codecov.yml file for code coverage
Add coverage support to Travis config
Small fixes to setup.py and tox config
Allow PYTHON to be set for Makefile
Unit test fix
Use snake_case for internal functions (no impact)
Add tests for registerSlot(), sendSignal() and unregisterSlot()
Add tests for makeCall(), registerCall() and returnCall()

See Travis for test results:
https://travis-ci.org/dagwieers/script.module.addon.signals
And Codecov for coverage results (we have 99% testing coverage!):
https://codecov.io/gh/dagwieers/script.module.addon.signals/branch/assorted-fixes/commits

dagwieers · 2019-12-16T01:49:09Z

I know this is quite a lot of unrelated changes. The only impacting change in this list is the move to using unicode_literals for Python 2 and related simplification to the code. That will need to be tested in Kodi with existing add-ons. I removed this for now

Let me know what parts you like me to split up in different PRs.

dagwieers · 2019-12-16T12:39:30Z

@ruuk There are two additional things I would like you to review specifically:

The docstrings for all methods
- I am not that familiar with the terminology of AddonSignals
Especially the registerCall(), returnCall() and makeCall() functions
- not sure why we have them (backward compatiblity?) and how they ought to be used
- I added integration tests, feel free to let me know what from those tests are useless

CastagnaIT · 2019-12-16T12:40:35Z

a curiosity question, i have seen that in all the projects you keep wrong the commas on the descriptions of the methods, why do you insert them all wrong?
so you use the wrong ''' instead of """

dagwieers · 2019-12-16T12:40:42Z

cc @mediaminister I always appreciate your input 😉

CastagnaIT · 2019-12-16T12:43:42Z

lib/AddonSignals.py

@@ -1,135 +1,162 @@
 # -*- coding: utf-8 -*-
+# GNU Lesser General Public License v2.1 (see COPYING or https://www.gnu.org/licenses/gpl-2.1.txt)


this not follow SPDX convention

Comments in code do not use SPDX. SPDX identifiers are used for computer-processed fields not for human-processed information (license one-liners are intended for humans).

I don't understand what you mean, but this does not follow the SPDX directives at all, if you do a test with various check software will not be approved.

You can see how to do in netflix repo,
even licenses should not be written with links, but placed in the folder LICENSES,
otherwise it is useless to follow the SPDX directives...

Make a new PR for your change if you like. I am not interested to take it this far.

The license is in LICENSE.txt, I don't see the point of an additional LICENSES/ directory if we have a single license anyway. (The license is 5x larger than the library, so having the license in the source tree twice is unneeded and unwanted IMO).

Again, conventions are nice, up to a point and then they become pedantic 😄

it is addon checker, devs want to go in this way...
agree that this project is big like a flea

AFAIK this was only related to addon.xml, not code comments in your source files.

dagwieers · 2019-12-16T12:47:19Z

so you use the wrong ''' instead of """

Why is using ''' wrong? There is no convention that places one before the other.

In Python I tend to use single quotes unless double quotes are required. I prefer single quotes because they are easier to read (less fluffy) then double quotes.

CastagnaIT · 2019-12-16T12:54:16Z

so understand, i prefer to use the correct ones otherwise PEP8 and pycharm complains...

dagwieers · 2020-05-04T11:41:37Z

I added GitHub CI support and removed Travis CI support. The unit tests now integrate better with GitHub and the output will look like this from the Actions tab at the top:
https://github.com/dagwieers/script.module.addon.signals/actions

It would be really nice if we can have the fixes and unit testing added.

ruuk · 2020-07-26T00:52:01Z

a curiosity question, i have seen that in all the projects you keep wrong the commas on the descriptions of the methods, why do you insert them all wrong?
so you use the wrong ''' instead of """

Because I started writing Python before one style was decided to be the 'right' style, and only recently started caring 😄.

ruuk

Sorry for the long, long delay in reviewing this. Changes look good, I'll merge after a rebase.

This PR includes: - Fix SPDX to LGPL-2.1-or-later - Switch to using unicode_literals for Python 2 - Add _to_unicode() function - Replace RECEIVER global with static variable - Replace getAddonInfo() call with _addon_id() using static variable - Remove unused CallHandler.data And also cosmetic/testing improvements: - Add docstrings to all methods and functions - Remove disabled import-error and missing-docstring pylint tests - A codecov.yml file for code coverage - Add coverage support to Travis config - Small fixes to setup.py and tox config - Allow PYTHON to be set for Makefile - Unit test fix - Use snake_case for internal functions (no impact)

This reduces the overhead of using AddonSignals at import time (as is most impacting on slower devices like Raspberry Pi). On Kodi all imports are performed at startup or at every invocation, even when not used.

This PR includes: - Fix a bug in the return signal name - Improve callback integration tests - Ensure receiver and sender use different SignalReceiver instances

This commit includes: - Add .gitattributes to get a proper ZIP pakage - Remove Travis CI - Add a GitHub CI workflow for unit testing, codecov and SonarCloud - Add a GitHub CI workflow for kodi-addon-checker - Move add-on icon to resources/

dagwieers · 2020-09-27T11:22:17Z

Ok, I noticed a new release without any tests and then found this PR still in the queue :-(

Rebasing this is real hell, because I lack the context 10 months later and I have to dig into what has changed since. So it takes a lot more effort than it could have been. Please next time, if a big PR is in the queue, do not merge anything else before this has been sorted out.

dagwieers · 2020-09-27T12:22:35Z

The current code is failing, where it used to work before:

$ python -m unittest discover -v
test_bogus_sender (tests.test_calls.TestCalls) ... Iteration: 1
Iteration: 2
Iteration: 3
Iteration: 4
Iteration: 5
ERROR
test_calls (tests.test_calls.TestCalls) ... ERROR
test_send_multiple (tests.test_calls.TestCalls) ... ERROR
test_unicode (tests.test_calls.TestCalls) ... ok
test_addon_signals (tests.test_encoding.TestEncoding) ... ok
test_bogus_sender (tests.test_signals.TestSignals) ... ERROR
test_register_multiple (tests.test_signals.TestSignals) ... Debug: {u'test': u'register_multiple'}
ok
test_send_multiple (tests.test_signals.TestSignals) ... ok
test_send_none (tests.test_signals.TestSignals) ... ok
test_send_nonsignal (tests.test_signals.TestSignals) ... ok
test_send_unregistered (tests.test_signals.TestSignals) ... ok
test_signals (tests.test_signals.TestSignals) ... ok
test_unicode (tests.test_signals.TestSignals) ... ok
test_unregister_nonexisting (tests.test_signals.TestSignals) ... ok
test_warning (tests.test_signals.TestSignals) ... Notice: ++++==== script.module.addon.signals: sourceID keyword is DEPRECATED - use source_id ====++++
ok

dagwieers · 2020-09-27T12:44:46Z

Ok, the new code returns OSError on abortRequested(). Not sure if this is the intended behaviour.
But after codifying this as the correct behaviour, the tests work as expected.

This is what it will look like in this project when merged: https://github.com/dagwieers/script.module.addon.signals/actions

lib/AddonSignals.py

dagwieers · 2020-09-27T12:50:51Z

I just added a GitHub CI release workflow.

If you add GH_USERNAME, GH_TOKEN and EMAIL, you can push a new release to GitHub and Kodi repositories by simply tagging a new release and pushing it to GitHub.

CastagnaIT · 2020-09-27T12:54:53Z

Ok, the new code returns OSError on abortRequested(). Not sure if this is the intended behaviour.
But after codifying this as the correct behaviour, the tests work as expected.

This is what it will look like in this project when merged: https://github.com/dagwieers/script.module.addon.signals/actions

now remember, yes the OSError raised is right behaviour
when Kodi stop the loop, the addon that have to receive the data
have to know when the operation is interrupted and stop his code

CastagnaIT · 2020-09-27T12:57:16Z

if you prefeer you can add a new custom exception for interrupted event but is not really necessary

dagwieers · 2020-09-27T12:58:06Z

Ok, the new code returns OSError on abortRequested(). Not sure if this is the intended behaviour.
But after codifying this as the correct behaviour, the tests work as expected.
This is what it will look like in this project when merged: https://github.com/dagwieers/script.module.addon.signals/actions

now remember, yes the OSError raised is right behaviour
when Kodi stop the loop, the addon that have to receive the data
have to know when the operation is interrupted and stop his code

Sure, but OSError seems to be a strange exception to be used for that. A custom exception, or maybe one from Kodi would make more sense IMO. A few months ago this was not handled, and did not raise an OSError.

The release workflow helps pushing releases to the Kodi repositories. Also add support starting from Gotham

CastagnaIT · 2020-09-27T13:00:00Z

if you want i think you can still add a custom exception
this code is not yet released

dagwieers · 2020-09-27T13:03:18Z

@CastagnaIT Sure, but I would leave that up to the person who wrote that ;-)
I would like to get this one merged first.

dagwieers · 2020-09-27T13:14:35Z

The coverage is no longer 99%, this is something to fix later IMO
https://codecov.io/gh/dagwieers/script.module.addon.signals/src/assorted-fixes/lib/AddonSignals.py

dagwieers force-pushed the assorted-fixes branch 9 times, most recently from 0c01791 to a4bd5a3 Compare December 16, 2019 01:44

dagwieers marked this pull request as ready for review December 16, 2019 02:00

dagwieers force-pushed the assorted-fixes branch 13 times, most recently from 48f8f54 to c607e19 Compare December 16, 2019 12:36

CastagnaIT reviewed Dec 16, 2019

View reviewed changes

dagwieers force-pushed the assorted-fixes branch from 0b0dbaa to 25f310d Compare May 4, 2020 11:38

ruuk self-requested a review July 26, 2020 01:13

ruuk approved these changes Jul 26, 2020

View reviewed changes

dagwieers added 7 commits September 27, 2020 12:57

Avoid loading libraries at import time

41c396f

This reduces the overhead of using AddonSignals at import time (as is most impacting on slower devices like Raspberry Pi). On Kodi all imports are performed at startup or at every invocation, even when not used.

Replace triple quotes by triple double quotes

f70513f

Add integration tests for calls

0fea64b

Clean up pylint some more

6558986

Fix a bug in the return signal

9417e6e

This PR includes: - Fix a bug in the return signal name - Improve callback integration tests - Ensure receiver and sender use different SignalReceiver instances

Move from Travis CI to GitHub CI

35493e4

This commit includes: - Add .gitattributes to get a proper ZIP pakage - Remove Travis CI - Add a GitHub CI workflow for unit testing, codecov and SonarCloud - Add a GitHub CI workflow for kodi-addon-checker - Move add-on icon to resources/

dagwieers force-pushed the assorted-fixes branch 2 times, most recently from 81c9f7e to 4f56683 Compare September 27, 2020 12:39

Fixes after rebase

23ba50c

dagwieers force-pushed the assorted-fixes branch from 4f56683 to 23ba50c Compare September 27, 2020 12:42

CastagnaIT reviewed Sep 27, 2020

View reviewed changes

lib/AddonSignals.py Show resolved Hide resolved

dagwieers force-pushed the assorted-fixes branch from 681acc1 to 1b7fef1 Compare September 27, 2020 12:55

Add release workflow

6a9cb6f

The release workflow helps pushing releases to the Kodi repositories. Also add support starting from Gotham

dagwieers force-pushed the assorted-fixes branch from 1b7fef1 to 6a9cb6f Compare September 27, 2020 13:00

		@@ -1,135 +1,162 @@
		# -- coding: utf-8 --
		# GNU Lesser General Public License v2.1 (see COPYING or https://www.gnu.org/licenses/gpl-2.1.txt)

Assorted fixes #9

Are you sure you want to change the base?

Assorted fixes #9

Uh oh!

Conversation

dagwieers commented Dec 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dagwieers commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dagwieers commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CastagnaIT commented Dec 16, 2019

Uh oh!

dagwieers commented Dec 16, 2019

Uh oh!

CastagnaIT Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

dagwieers Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

CastagnaIT Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

dagwieers Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

CastagnaIT Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

dagwieers Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

dagwieers commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CastagnaIT commented Dec 16, 2019

Uh oh!

dagwieers commented May 4, 2020

Uh oh!

ruuk commented Jul 26, 2020

Uh oh!

ruuk left a comment

Choose a reason for hiding this comment

Uh oh!

dagwieers commented Sep 27, 2020

Uh oh!

dagwieers commented Sep 27, 2020

Uh oh!

dagwieers commented Sep 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dagwieers commented Sep 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CastagnaIT commented Sep 27, 2020

Uh oh!

CastagnaIT commented Sep 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dagwieers commented Sep 27, 2020

Uh oh!

CastagnaIT commented Sep 27, 2020

Uh oh!

dagwieers commented Sep 27, 2020

Uh oh!

dagwieers commented Sep 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dagwieers commented Dec 15, 2019 •

edited

Loading

dagwieers commented Dec 16, 2019 •

edited

Loading

dagwieers commented Dec 16, 2019 •

edited

Loading

dagwieers commented Dec 16, 2019 •

edited

Loading

dagwieers commented Sep 27, 2020 •

edited

Loading

dagwieers commented Sep 27, 2020 •

edited

Loading

CastagnaIT commented Sep 27, 2020 •

edited

Loading