Ignore the log files when they are generated

xbmc / addon-check

Automatic checks for new repository submissions

GNU General Public License v3.0

52 stars 42 forks source link

Ignore the log files when they are generated #236

Closed thombet closed 4 years ago

thombet commented 4 years ago

Fixes #233

Create two functions in the common module to get the full path of the log files (debug log and reporter log) anywhere in the script. It avoids duplicate reference to the names of the log files to keep the code DRY.
When a log file is generated it will be whitelisted to avoid reporting warnings.
Files with the same name as the log files but not in the current folder will still be reported (these files were generated from a previous run of the checker so they should not be ignored)

thombet commented 4 years ago

Some additional information:

It seems both .format() and % are used in the code but I used % in __main___.py to be consistent with the rest of this file.
From a design point of view I think it would have been better to define get_reporter_log_path() in the LogReporter class (plugins/log_reporter.py) but I didn't find a good way to use this class accross the code since it is imported with load_plugins() in __main__.py. If you have a better idea, feel free to suggest it :)
~~I ran several (manual) tests locally to confirm it is working as expected but should I create any auto tests?~~ See new commits below

thombet commented 4 years ago

The tests failed because the stub of argParser does not work anymore with my change. I will fix this and probably add some tests to reproduce the use cases I tested manually.

enen92 commented 4 years ago

There are a lot of use cases for this tool, namely via pipelines. I am not sure I like the idea of ignoring specific files that may end up bundled with the addon submission. Maybe a possible option is to only allow usage of --log or --debug log if an additional argument (log path) is submitted? Maybe also error out if the submitted log path is equal to the directory we are checking for validation?

Another option is to make it platform dependent and make it log by default to the standard log location per platform (/var/log/addon-check on linux, ~/Library/Logs/ on osx, etc) and at the end of the execution state the log can be found in xxxx ?

thombet commented 4 years ago

I came up with this solution because I thought requesting a path for the log files would break the compatibility with previous versions but your idea of using a default path solves this 🙂

I agree this PR adds a risk of submitting ignored files when used in pipelines so I will push a new update implementing your ideas. Thank you for suggesting them @enen92 👍

razzeee commented 4 years ago

Should we close this until you find time to work on this again?

thombet commented 4 years ago

Should we close this until you find time to work on this again?

I can't believe it's been 4 months already... Yes I will close this pull request and I will upload a new one later, thanks for the reminder.