Add Pylint logs #512
Add Pylint logs #512
Conversation
[PYB-01] Create print logs
| errors_info = '' | ||
| warnings = 0 | ||
| warnings_info = '' | ||
| show_info_messages = project.get_property('pylint_show_info_messages') |
There was a problem hiding this comment.
Do we really need separated properties pylint_show_info_messages and pylint_show_warning_messages? Could we use global project property project.get_property('verbose') instead?
There was a problem hiding this comment.
With verbose you can not decide if you want to see only warnings, moreover with verbose you will see a lot of other logs. I think it is good to have separate configuration for this.
What do you think about this @AlexeySanko ?
There was a problem hiding this comment.
Hi, @maciejpolanczyk
I used idea that pylint shows all issues anyway. And verbose mode supposes that output could be big. 😁
There was a problem hiding this comment.
pylint shows all issues in report file. I would like to have possibility to show only warnings or only infos from pylint on console as output from pybuilder without big output which will be created by 'verbose' from other plugins and pybuiilder. It will be easy to read them and I will not have to go to report file to find out what went wrong.
What do you think about it @AlexeySanko ? Or should we abandon this idea because it is not consistent with unit tests execution?
There was a problem hiding this comment.
On the other hand - cause of report parsing You can manage different type of issues. Why not?
There was a problem hiding this comment.
Do you mean all logger.info in this file or only the one which shows infos from pylint?
We decided for logger.info base on other plugins which use this level for printing information which should be print in regular run (without verbose)
There was a problem hiding this comment.
Moved to particular row - https://github.com/pybuilder/pybuilder/pull/512/files#r148052635
| @@ -44,4 +45,63 @@ def execute_pylint(project, logger): | |||
| logger.info("Executing pylint on project sources") | |||
|
|
|||
| command_and_arguments = ["pylint"] + project.get_property("pylint_options") | |||
| execute_tool_on_modules(project, "pylint", command_and_arguments, True) | |||
| pylint_output_file_path = execute_tool_on_modules(project, "pylint", command_and_arguments, True)[1] | |||
There was a problem hiding this comment.
I suggest to use
execute_tool_on_source_files(project, "pylint", command_and_arguments, include_test_sources=?)
We will get two advantages:
- provide possibility to include test_sources with property
pylint_include_test_sources - remove possible problems with pylint:
execute_tool_on_modulesusesdiscover_moduleswhich returns list of all packages AND modules. For example, I have packagepckgwith modulesa,bandc.discover_modulesreturns list:[ 'pckg', 'pckg.a', 'pckg.b', 'pckg.c']and pylint will report problem with duplicated code into files (it will parsepckg.atwice).
| warnings_info = '' | ||
| show_info_messages = project.get_property('pylint_show_info_messages') | ||
| show_warning_messages = project.get_property('pylint_show_warning_messages') | ||
| for line in file_content: |
There was a problem hiding this comment.
If we parse pylint results I think that more suitable is JSON format --output-format=json instead of raw text. It's more resistant to future changes, isn't?
Example, of JSON output
[
{
"message": "Missing module docstring",
"obj": "",
"column": 0,
"path": "src/main/python/some_lib/model.py",
"line": 1,
"type": "convention",
"symbol": "missing-docstring",
"module": "some_lib.model"
},
{
"message": "Invalid constant name \"lc\"",
"obj": "",
"column": 0,
"path": "src/main/python/some_lib/model.py",
"line": 12,
"type": "convention",
"symbol": "invalid-name",
"module": "some_lib.model"
}
...
There was a problem hiding this comment.
Unfortunately, JSON report doesn't provide scores. :(
| 'Pylint ratio: {} / Pylint change: {}'.format(pylint_score, pylint_change) + errors_info + warnings_info | ||
| ) | ||
|
|
||
| if errors > 0 and project.get_property('pylint_break_build_on_errors'): |
There was a problem hiding this comment.
flake8 and frosted use *_break_build property. I suggest to do the same: use pylint_break_build
There was a problem hiding this comment.
Hi @AlexeySanko thanks for quick response to PR. In case of pylint we have a little different situation. In this plugin we assumed we can fail from different reasons:
- errors in output
- pylint score too low
- pylint change of score (for example from 9.0 to 8.5) is too big
This is why we used 'pylint_break_build_on_errors' for the first case.
Using 'pylint_break_build' suggests this is global brake/not brake which is not true in this case.
Do you agree with this approach? Do you have suggestion for better naming in this case?
There was a problem hiding this comment.
Hi, @maciejpolanczyk
I thought that You will not support RP and created external plugin pybuilder_pylint_extended.
I used idea that plugin has to break build on errors and fatals anyway - otherwise You can disable and do not use plugin. Errors are errors and have to be fixed.
There was a problem hiding this comment.
Good point, we will change it to work as you described.
| warnings = 0 | ||
| warnings_info = '' | ||
| show_info_messages = project.get_property('pylint_show_info_messages') | ||
| show_warning_messages = project.get_property('pylint_show_warning_messages') |
There was a problem hiding this comment.
I could set show_info_messages = True and show_info_messages = True when verbose and debug are true. What do you think about it @AlexeySanko ?
There was a problem hiding this comment.
Hi, @maciejpolanczyk
Good idea. Thanks!
| warnings += 1 | ||
|
|
||
| if show_info_messages and (line.startswith('C:') or line.startswith('R:')): | ||
| logger.info('Pylint: Module %s: ' % module_name + line) |
There was a problem hiding this comment.
Hi, @maciejpolanczyk
Info - summary and common build information. More looks like that convensions and refactorings should be pass to logger.warn too.
There was a problem hiding this comment.
I agree, will fix this.
Pylint logs are shown in shell. Break of building in case of too low Pylint score or too high Pylint decrease added. Also build break on Pylint errors with option to switch it off.