Improve addon report infrastructure

[slgobinath]

Hi @razzeee,
Could you please have a look at this design improvement. This PR improves how addon-checker handles reports. Currently, all reports are printed to the console as you check add-ons. It cannot support multiple reporters (say GitHub reporter, markdown reporter, checkstyle reporter, etc).

This PR isolates reports and reporters into separate modules so that add-on checker can be easily extended to support new reporters in future. For the moment, I have implemented ConsoleReporter which is printing the report to the console as we have right now.

The low level Record object is designed to include line numbers and character level positions for future requirements (to report errors in Python scripts). This design also allows you to report problems and/or warnings at repo level, add-on level or file level.

I have also defined three log levels: 'INFO', 'WARN' and 'ERROR'(which is equivalent to 'PROBLEM'). These terms were chosen to make the output more structured.

Sample report generated according to this implementation:

Checking /home/gobinath/Workspace/slgobinath/kodi/repo-plugins

Checking plugin.audio.detektorfm
INFO: Created by sorax
INFO: Addon id matches folder name
INFO: Image icon exists
INFO: icon dimensions are fine 512x512
INFO: Image fanart exists
INFO: fanart dimensions are fine 1920x1080
WARN: Empty image tag found for screenshot

Checking plugin.audio.eco99music
INFO: Created by Nir Elbaz
INFO: Addon id matches folder name
INFO: Image icon exists
INFO: icon dimensions are fine 256x256
INFO: Image fanart exists
INFO: fanart dimensions are fine 1920x1080
INFO: Image screenshot exists
INFO: Image screenshot exists
INFO: Image screenshot exists

Checking plugin.audio.hearthis_at
INFO: Created by Eike Baran
INFO: Addon id matches folder name
INFO: Image icon exists
INFO: icon dimensions are fine 512x512
INFO: Image fanart exists
INFO: fanart dimensions are fine 1920x1080
INFO: Image screenshot exists
INFO: Image screenshot exists
INFO: Image screenshot exists

Annotation support is introduced for future implementations to automate config property definition which will be used to enable/disable reporters using .test-config.json or command line argument.

WDYT?

[enen92]

I like very much the idea, at least it decouples reporting from the checking step which is really usefull for creating integrations later. Two minor suggestions:

1. Try to avoid log as the method name. At first it made me think your reporters were extensions of the python logging module when in fact they are custom implementations. Use report for example.

2. Try to avoid importing reporters directly in checkers like your are doing right now. The checker should be agnostic to what reporters are available. That being said, and since decorators are executed when you import a module it would make more sense to define the import logic in the __init__.py file of the reporter module using the __all___ variable for example. By doing this (or a similar logic) you are creating a sort of "ReporterManager" which holds references to all available reporters. The manager would also be responsible for ignoring/selecting reporters if they were somehow defined in a configuration file.

[slgobinath]

Hi @enen92,

Try to avoid log as the method name

Sure

Try to avoid importing reporters directly in checkers like your are doing right now

You have read my mind :-)
That is the plan. This PR is to get an approval. I will implement ReportManager, ConfigManager and any other required modules to automate importing reporters and enable/disable them depending on configuration. The decorator added in this PR is for that purpose but not yet used for automatic injection.
I will work on that.

[enen92]

Well, it definitely has my approval. That's in fact one of the reasons for opening #46 . The issue is not specifically targetted at Checkstyle but more at architectural considerations to decouple the checking and reporting steps so that external tools can consume data produced by the tool and maybe be integrated into IDE's, text editors or code quality projects. Great to see this being implemented at such an early stage to avoid introducing drastic changes in the future

[razzeee]

Very very good direction to move things in. So definatly welcome. Keep up the great work. 🎉

[slgobinath]

Please check the new commit.

[slgobinath]

Hi @enen92,
I tried renaming the function name log to report but then it looks odd when we call it using Report object which is named report
For example:

report.report(Record(INFORMATION, "Created by %s" % addon.attrib.get("provider-name")))

Shall we find a different name or leave log for the moment? WDYT?

[razzeee]

Add might work, as we basically add the record / report item and do some processing

[slgobinath]

Renamed log to add.
With the commit cd194c4, reporters are injected and managed automatically. A new configuration reporter is added to both .test-config.json and command line arguments which are used to enable reporters.

By default, Console reporter is enabled (@reporter(name="console", enabled=True)). If you provide "reporter": [] in .test-config.json as below, it will disable all reporters.

{
    "check_legacy_language_path": true,
    "check_legacy_strings_xml": true,
    "check_license_file_exists": true,
    "reporter": []
}

The user can override that option in the command line by providing --reporter console. The reporter name console is determined by the decorator @reporter(name='console'...). Therefore, if you add a new reporter, you just need to define the name and whether it is enabled by default.

$ kodi-addon-checker --help
usage: kodi-addon-checker [-h] [--version] [--reporter {console}]
                          [add-on [add-on ...]]

Checks Kodi repo for best practices and creates problem and warning reports.
If optional add-on directories are provided, check only those add-ons.
Otherwise, scan current repository and check all add-ons in the current
directory.

positional arguments:
  add-on                optional add-on directories

optional arguments:
  -h, --help            show this help message and exit
  --version             show program's version number and exit
  --reporter {console}  enable a reporter with the given name. You can use
                        this option multiple times to enable more than one
                        reporters

If there are more than one reporters (say console and github) user can enable both of them using the following command:

kodi-addon-checker --reporter console --reporter github

It can also be achieved by modifying .test-config.json as below:

{
    ...
    "reporter": ["console", "github"]
}

I feel it is enough for a PR. I will improve the file checker architecture in another PR. Please let me know if there is anything to fix.

[razzeee]

All good code wise, but I want to do some manual tests before merging.

[razzeee]

Well maybe it's not needed / not a good fit to derrive Record from Report?

[slgobinath]

@razzeee, please note that the inheritance has been removed in d1235ed

[razzeee]

I'll try to review later today, thank you!

[razzeee]

@slgobinath I just added some suggestions.
Do you think it's better without the inheritance? Just curious, what you think.

[slgobinath]

@razzeee please note that I have fixed all your suggestions in slgobinath@8fb6fc0

Do you think it's better without the inheritance?

I am from strong OOP and strict type background (Java person) so I was too concern about exact relationships. My design was "Record IS A Report" and "Report HAS A Report". However, I find removing inheritance makes the code more readable and does not hurt in Python world so +1 for it.

[razzeee]

Thank you very much for this 🎉
Really like the comments you added in the last commit.