[mzfr]

Hi,

I’m Mehtab Zafar, a CS undergrad from Delhi - India. I’m interested in working on Kodi’s addon checker idea during GSoC.

For the past two weeks I’ve been contributing to the addon-check tool and have a good handle on the problem that we’re trying to solve. I’ve also been researching what my GSoC proposal would look like.

Broadly, the goal of the project is to reduce the workload of addon reviewers by automating as much work as possible and setting a high bar for the code quality of addons.

Kodi’s roadmap seems to be pushing for Python 3 with Kodi v19 being planned to be Python 3 only. As we already have working Python 3 support (due to arpit’s last year GSoC project.), the main focus is now on porting all the addon code to Py3 as well. One of the ways to achieve this is to add python 3 detection in the addon-check tool so that addon creators find out the issues and can gradually fix them. I’ve created a separate issue for this on the repo suggesting an approach.

Apart from addons being Python 3 compatible, we also want them to be of good quality which we will do by static code analysis.

Code analysis can be divided into two broad types - Python specific checks, or general code quality metrics:
 

• For python specific things like PEP8 style etc. we could use PyLint which seems to cover everything from pep8 to error detection (and even refactoring etc.) It is also fully customisable via a config file.
  
  
• As far as generic code quality metrics are concerned - radon (& xenon) seems to be the tool of choice. It has the usual metrics like McCabe’s cyclomatic complexity etc. and a bunch of other ones.
  

Just as a concrete example, I ran PyLint on the YouTube Kodi Addon which found some minor issues and gave it a rating of 7/10 showing that code is generally of good quality, while running it on Reddit Viewer gave a rating of -1/10. Both of these addons turned out to have similar Cyclomatic Complexity: 2.8 (for youtube) and 4 (for reddit.)

Tools like PyLint are more actionable as there is, for most of the cases, a direct way to fix the error being reported, but the same is not true for complexity metrics so reducing them might involve restructuring the entire code and will need a lot of effort on the part of addon developer.

Another thing to keep in mind with these metrics is that addon developers might be hobbyists so they might not really be familiar with these technical terms (I only learnt about some of them in a Software Engineering course I took.) So if we’re planning to add them, we’ll have to do some work on making the developers understand them as well.

This is what I’ve been able to think about the project till now. I’m also looking into what other projects use python addons and how they handle code review etc. I’ll post my findings here. Till then, I’d love some review/feedback on my approach.

[Razze]

Hey Mehtab, good to have you here!

I really like your write up. And I feel like we should focus on ways to improve code quality over time and not make this essential for everybody. So even PEP8 style, while I would love to have everything in it, I don't think we should force everybody to adopt it. If somebody can change my mind on that please do
It all boils down to not blocking addon submissions for bad style. Just encourage them to improve.
But sound the alarm for dangerous code or broken code (as in not running). These are must fixes.

Will ultimatly be mostly up to the repo maintainers, but it's something we need to have in the back of our heads.

[mzfr]

Hi @Razze 
Right now for the travis-comments issue we are waiting for the updates from bluzi's side. Until then it would be nice if you can point out some other things in my draft or if everything is fine should I just work on other issues.

[Razze]

Your propsal is fine, so I can't point at anything
If you want to, you can work on other things

[mzfr]

@Razze definitely i want to work on other things.
There are few issues that requires help like
For Schemas I am waiting for a reply from @tamland
For file permission issue I am waiting for @martinkaijser reply.
For dependency issue again I waiting for some guidance.
I would really appreciate if you can help me with any of those things.

[Razze]

I updated the dependency issue

[mzfr]

[Update - Week 1]

I am currently working on one of my old Pull requests In this I have added a complexity check for the addon's entry point files.
I'm also looking into further improving the existing code structure - by moving related functions to separate modules etc.

[mzfr]

[Update - week 2]

1) Got the old pull merged.
2) Worked on an another pull request - It checks for dependencies present in addon.xml and reports them if old version for any dependency is used in addon.
3) Came up with the issue - check for __init__.py file.  - This is a problem because pylint would cause a problem if their isn't any __init__.py in the folder. But the problem in this is that almost every addon is missing this file so the check would   always fail on this. Haven't discussed much with mentor on this.
4) I have started working on refactoring code. Will separate similar type of function into single file. Also I will break the complex functions into multiple function e.g _check_image_type

[mzfr]

[Update - week 3]

1) Completed work on an old pull request.

2) After discussion about the Pylint issue we decided not to go with some vague fixes and plans to fix the issue within pylint. I have already done some work on the fix. Now I only have to tie everything together.

3) In last update I mentioned that I will be starting work on refactoring the code but instead of doing that @razzeee asked me to write some tests for the code before refactoring. This will help us to make sure that refactoring doesn't introduce any regressions and we don't end up changing the the behavior of our existing code.
So I have started writing integration test for it.

And I also I will not be able to work this week[14-22 may] because of my exams. But after that I will be totally free and will dedicate all my time toward the project.

[mzfr]

[Update - Week 5]

1) Created a new test addon for testing. This addon fails on some checks and passes on some other checks.

2)  Most of my time this week was spend on working on PR #57. My work is done on the PR and soon it will be merged. This feature will help us to check whether the newly submitted addon already exists in old branches(gotham, helix etc) or not. It will also check whether the old addons are marked broken or not. This will help in only accepting new addons or getting old addons but only with a bumped version.

3) I created a new PR #74 for checking multiple artworks(icons/fanart) but this was not supposed to be done the way I did it. This will have to be done XML schemaso I'll do it that way.

4) PR #73 - Remove multiple parsing of addon.xml: we were parsing the same addon.xml file multiple times. So this PR will remove those multiple parsing and instead of that we'll parse it just once and use that parsed element in other functions that requires it.

5) Thanks to @tamland for providing examples on completing the schemas(PR #35). Now I know how to do it but I think I will not be working on them, atleat not this week.


Goals for coming week:

1) The moment #57 get merge I'll submit a new PR for integration test. I have already done some work on them but I'll have to tweak them according to the changes done in #57.

2) Refactor the code. This is very important part of my project and this thing is needed to be done as early as possible because continuos addition of new feature will make this work more hectic. So right after completing my work on test, I'll start refactoring the code.

[mzfr]

[Update - week 6]

1) Created a logger PR #81 (merged) - We were having a hard time debugging our code. So I created a logger that will help us log all the exception/problems without messing up the travis-ci logs

2) Add check for existing addon PR #57 (merged) - This PR was supposed to be merged in the starting of the week but then we faced issue realted to debugging which in return led to creating a logger. And also we were having issues related request timeouts.

3) Integration test PR #84 - I have submitted the PR but there are some small changes required in it. These test will cover the complete kodi-addon-checker tool and will help us maintain the nature of the codebase

This week the work done was at a bit slow rate but I'll try to do my best for the coming week.

Targets for coming week:

1) Complete the work on PR #84.
2) Refactor the code, submit a PR for it and try to get it merged.
3) Work on issue #82 - handle entry point validation for resource addons and issue #83 ignore pseudo addons in our checks

[mzfr]

[Update - week 7]

1) Created PR #87 (merged) - Moved all_repo_addons function out of __main__.py. So we were having issues with imports being in __init__.py file for integration test and @enen92 pointed out that we shouldn't be calling any function from __main__.py file, it should be for entry. So I moved the `all_repo_addon function` out of the __main__.py file.

2) Check complex entry point for xml file - PR #86 (merged) so this was the fix for issue #82. We already had checks for entry point files but they were specifically for `.py` files but resource addon happens to have `.xml` files as their entry point so it broke tests for resoure addon.

3) updated ignore list of pseudo addons - PR #88 (merged):This was fix for issue #83. We have a list of addons we don't want those to be counted as dependencies so we added them in ignore list. Now this list was updated by @Razze so I added them to our code base.

4) Test for kodi-addon-checker - PR #84 (merged) Even though I submitted this PR 7 days ago but I did some changes to this PR. My code was a bit hard to understand as I used multiple list comprehension which made it complex and for some reason I was not able to break it down into multiple lines so it took sometime to get this PR merged. But now we have whole integrated test for our tool.

5) Add PR argument - PR #90 (merged): There was some issue with one the functions(check_existing_addons) it was having a different behavior then what we wanted. And later it was decided that we should keep the current behavior for non PR checks and should add a different behavior for PR checks. Now the way to do this to use Travis_ci variable but we have had some problem in past with travis-ci. So I added a new command line flag for handling PR/Non-PR checks.

6) Catch exception per addon for repo checks - PR #92 (merged): addon-checker was crashing on repo-checks just because a single addon was having some problems/error, which was not good. so @Rechi pointed out that we should do some exception handling for single addons in whole repo check.

7) Refactoring codebase - PR #89: So finally I've re-factored our codebase. Moved all the code from one file(check_addon.py) into multiple files. The stats were not very fascinating but there was improvement specially in `_check_image_type` function but all in all the code base after this will be lot easy to understand and maintain. The PR is currently reviewing process.


Targets for coming week:

1) Add radon check - I have the basic plan but I'll have a discussion with mentors about what kind of output we want from this check and then I'll work on this.

2) I am currently working on fixing the pylint issue but I was having some issues with it so I asked @PCManticore, one of the member of python code quality authority, but haven't got any reply from him yet. So if I get something from him I'll work on fixing the issue #352 on pylint.

[mzfr]

[Update - Week 8]

1) got the PR #89 for refactoring merged. The thing I did was correct but I was having a lots of issue with commits and rebasing the branch. All because of my stupidity of making a one big commit. But thank to @Rechi for being so patient with me and guiding me through the whole mess.

2) Made two small PRs:
    - Removed unused variables and imports - PR #95 - there were unused variables after refactoring so removed those
    - change normal imports to relative imports - PR #94 - we were using long imports like `from kodi_addon_checker import xyz`
            so I chaneged them into relative imports i.e `from . import xyz` for safety in future.

3) Created two PR related to Pylint issue:
    - Add option for running  pylint without __init__ file - PR #2190 on pylint - Basically we are trying to make pylint run without looking for the presence of __init__.py file. So I am working on the fix and this PR will add a optional flag on whether to run tool with or without checking ofr __init__.py file.

    - changes for running pylint wihtout __init__  file - PR #560  on astroid - Pylint works with astroid so there were some small chaneges required there.

Plans for this week:

I am planning to just work on the pylint issue because after discussing, mentors want this implementation first so will continue to work on it. Pylint is quite big and fixing one thing lead to breaking other things. Also I am getting some help from @PCManticore of pylint and even @Razze said he'll try to help whenever he's free so I am hoping this week I'll get this done.

[mzfr]

[Update - Week 9]

1) PR #97 - very small PR for updating the ignore list for dependencies

2) I was finally able to fix the pylint `__init__.py` file issue(PR #2190). It took most of my time this week but after getting a bit familiar with the pylint codebase I was able to fix the problem.
But now the problem I am facing with pylint is in writing test cases for the code that I have changed. I have tried a lot but they seem to be failing all the time and I can't understand why is that happening, I have followed the same trend of other pylint tests but still I wasn't successfull in writing those tests. @Razze also had a look and there seems some problem with the assertion but still it's not clear what the actual problem is. I have informed @PCManticore about the problem I am facing via Github comment but haven't got any reply from him so soon I'll write an email for asking some help from him.

3) During the time I was waiting for the reply from @PCManticore, I started implementing pylint integration and I am half way through it so right after the pylint issue is officially fixed and we gets a new release in our hand then it won't take much time to have pylint checker for our tool.

[docwra]

Thanks for the updates, really appreciate hearing about the work on this project.

Also great to see upstream getting an improvement! Just goes to show how great open source is when people work together.