Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Pylint logs #512

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add Pylint logs #512

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
56c2a9c
[PYB_01] Pylint plugin shows logs in shell and breaks build on certai…
michalfrak Oct 12, 2017
79c85ec
[PYB_01] Unit tests
michalfrak Oct 13, 2017
01c9c59
[PYB_01] Code review - patch over class
michalfrak Oct 18, 2017
dfc9b09
[PYB_01] Code review - execution separated from assertion
michalfrak Oct 18, 2017
7886ae6
[PYB_01] Code review
michalfrak Oct 18, 2017
3d82cc7
Merge pull request #2 from michalfrak/PYB_01_Create_pylint_logs
michalfrak Oct 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
64 changes: 62 additions & 2 deletions src/main/python/pybuilder/plugins/python/pylint_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@
# limitations under the License.

from pybuilder.core import use_plugin, after, init, task
from pybuilder.utils import assert_can_execute
from pybuilder.errors import BuildFailedException
from pybuilder.plugins.python.python_plugin_helper import execute_tool_on_modules
from pybuilder.utils import assert_can_execute

use_plugin("python.core")
use_plugin("analysis")
Expand All @@ -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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to use

execute_tool_on_source_files(project, "pylint", command_and_arguments, include_test_sources=?)

We will get two advantages:

  1. provide possibility to include test_sources with property pylint_include_test_sources
  2. remove possible problems with pylint: execute_tool_on_modules uses discover_modules which returns list of all packages AND modules. For example, I have package pckg with modules a, b and c. discover_modules returns list: [ 'pckg', 'pckg.a', 'pckg.b', 'pckg.c'] and pylint will report problem with duplicated code into files (it will parse pckg.a twice).


with open(pylint_output_file_path, 'r') as f:
file_content = f.read().splitlines()

module_name = ''
pylint_score = 0
pylint_change = 0
errors = 0
errors_info = ''
warnings = 0
warnings_info = ''
show_info_messages = project.get_property('pylint_show_info_messages')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @maciejpolanczyk
I used idea that pylint shows all issues anyway. And verbose mode supposes that output could be big. 😁

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand - cause of report parsing You can manage different type of issues. Why not?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to particular row - https://github.com/pybuilder/pybuilder/pull/512/files#r148052635

show_warning_messages = project.get_property('pylint_show_warning_messages')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @maciejpolanczyk
Good idea. Thanks!

for line in file_content:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
    }
...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, JSON report doesn't provide scores. :(

if line.startswith('************* Module'):
module_name = line.split(' ')[2]

if line.startswith('E:') or line.startswith('F:'):
logger.error('Pylint: Module %s: ' % module_name + line)
errors += 1

if show_warning_messages and line.startswith('W:'):
logger.warn('Pylint: Module %s: ' % module_name + line)
warnings += 1

if show_info_messages and (line.startswith('C:') or line.startswith('R:')):
logger.info('Pylint: Module %s: ' % module_name + line)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @maciejpolanczyk
Info - summary and common build information. More looks like that convensions and refactorings should be pass to logger.warn too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, will fix this.


if line.startswith('Your code has been rated at '):
pylint_score = float(line.split(' ')[6].split('/')[0])
pylint_change = float(line.split(' ')[10][:-1])

if errors > 0:
errors_info = ' / Errors: {}'.format(errors)

if warnings > 0:
warnings_info = ' / Warnings {}'.format(warnings)

logger.info(
'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'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flake8 and frosted use *_break_build property. I suggest to do the same: use pylint_break_build

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, we will change it to work as you described.

raise BuildFailedException(
"Pylint: Building failed due to {} errors or fatal errors".format(errors)
)

pylint_expected_score = project.get_property('pylint_score_threshold')
pylint_expected_score_change = project.get_property('pylint_score_change_threshold')
if pylint_expected_score and pylint_score < pylint_expected_score:
raise BuildFailedException(
"Pylint: Building failed due to Pylint score({}) less then expected({})".
format(pylint_score, pylint_expected_score)
)
if pylint_expected_score_change and pylint_change < pylint_expected_score_change:
raise BuildFailedException(
"Pylint: Building failed due to Pylint score decrease({}) higher then allowed({})".
format(pylint_change, pylint_expected_score_change)
)
78 changes: 57 additions & 21 deletions src/unittest/python/plugins/python/pylint_plugin_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,45 +15,81 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

from unittest import TestCase
from test_utils import Mock, patch
from logging import Logger
from tempfile import NamedTemporaryFile
from unittest import TestCase

from pybuilder.core import Project
from pybuilder.errors import BuildFailedException
from pybuilder.plugins.python.pylint_plugin import (check_pylint_availability,
init_pylint,
execute_pylint,
DEFAULT_PYLINT_OPTIONS)
from test_utils import Mock, patch


@patch('pybuilder.plugins.python.pylint_plugin.execute_tool_on_modules')
class PylintPluginTests(TestCase):

def setUp(self):
self.project = Project(".")
init_pylint(self.project)
self.mock_logger = Mock(Logger)

@patch('pybuilder.plugins.python.pylint_plugin.assert_can_execute')
def test_should_check_that_pylint_can_be_executed(self, mock_assert_can_execute):
def test_should_check_that_pylint_can_be_executed(self, mock_assert_can_execute, mock_execute_tool):
check_pylint_availability(self.mock_logger)
expected_command_line = ('pylint',)
mock_assert_can_execute.assert_called_with(expected_command_line, 'pylint', 'plugin python.pylint')

mock_logger = Mock(Logger)
def test_should_run_pylint_with_default_options(self, mock_execute_tool):
self._create_pylint_log_file('E:476, 8: Error message')
self._run_execute_tool(mock_execute_tool)
mock_execute_tool.assert_called_with(self.project, "pylint", ["pylint"] + DEFAULT_PYLINT_OPTIONS, True)

check_pylint_availability(mock_logger)
def test_should_run_pylint_with_custom_options(self, mock_execute_tool):
self._create_pylint_log_file('E:476, 8: Error message')
self.project.set_property("pylint_options", ["--test", "-f", "--x=y"])
self._run_execute_tool(mock_execute_tool)
mock_execute_tool.assert_called_with(self.project, "pylint", ["pylint", "--test", "-f", "--x=y"], True)

expected_command_line = ('pylint',)
mock_assert_can_execute.assert_called_with(expected_command_line, 'pylint', 'plugin python.pylint')
def test_should_show_error_message_in_pyb_logs(self, mock_execute_tool):
self._create_pylint_log_file('E:476, 8: Error message')
self._run_execute_tool(mock_execute_tool)
self.mock_logger.error.assert_called_with('Pylint: Module : E:476, 8: Error message')

@patch('pybuilder.plugins.python.pylint_plugin.execute_tool_on_modules')
def test_should_run_pylint_with_default_options(self, execute_tool):
project = Project(".")
init_pylint(project)
def test_should_show_warning_message_in_pyb_logs(self, mock_execute_tool):
self._create_pylint_log_file('W:2476, 8: Warning message')
self.project.set_property('pylint_show_warning_messages', True)
self._run_execute_tool(mock_execute_tool)
self.mock_logger.warn.assert_called_with('Pylint: Module : W:2476, 8: Warning message')

execute_pylint(project, Mock(Logger))
def test_should_break_build_on_errors(self, mock_execute_tool):
self._create_pylint_log_file('E:2476, 8: Warning message')
self.project.set_property('pylint_break_build_on_errors', True)
expected_message = 'Pylint: Building failed due to 1 errors or fatal errors'
with self.assertRaisesRegexp(BuildFailedException, expected_message):
self._run_execute_tool(mock_execute_tool)

execute_tool.assert_called_with(project, "pylint", ["pylint"] + DEFAULT_PYLINT_OPTIONS, True)
def test_should_break_build_on_too_low_pylint_score(self, mock_execute_tool):
self._create_pylint_log_file('Your code has been rated at 4.60/10 (previous run: 4.60/10, +0.00)')
self.project.set_property('pylint_score_threshold', 5.0)
expected_message = 'Pylint: Building failed due to Pylint score\(4.6\) less then expected\(5.0\)'
with self.assertRaisesRegexp(BuildFailedException, expected_message):
self._run_execute_tool(mock_execute_tool)

@patch('pybuilder.plugins.python.pylint_plugin.execute_tool_on_modules')
def test_should_run_pylint_with_custom_options(self, execute_tool):
project = Project(".")
init_pylint(project)
project.set_property("pylint_options", ["--test", "-f", "--x=y"])
def test_should_break_build_on_too_high_pylint_score_decrease(self, mock_execute_tool):
self._create_pylint_log_file('Your code has been rated at 4.60/10 (previous run: 6.60/10, -2.00)')
self.project.set_property('pylint_score_change_threshold', -1.0)
expected_message = 'Pylint: Building failed due to Pylint score decrease\(-2.0\) higher then allowed\(-1.0\)'
with self.assertRaisesRegexp(BuildFailedException, expected_message):
self._run_execute_tool(mock_execute_tool)

execute_pylint(project, Mock(Logger))
def _create_pylint_log_file(self, test_file_content):
self.temp_file = NamedTemporaryFile()
with open(self.temp_file.name, 'w') as f:
f.write(test_file_content)

execute_tool.assert_called_with(project, "pylint", ["pylint", "--test", "-f", "--x=y"], True)
def _run_execute_tool(self, mock_execute_tool):
mock_execute_tool.return_value = (0, self.temp_file.name)
execute_pylint(self.project, self.mock_logger)