Refactor - Burst utils.py into a "utils" and "message" package #2654

Pierre-Sassoulas · 2018-12-18T23:06:15Z

utils.py was big enough that is could cause issue. This is done in order to reduce saving time with syntax aware IDE, and also to make finding a piece of code easier for developer. I think this PR could make development on utils.py easier in the future.

coveralls · 2018-12-18T23:22:36Z

Coverage increased (+0.03%) to 89.742% when pulling 757b2a5 on Pierre-Sassoulas:utils_refactor into b18acac on PyCQA:master.

pylint/utils/utils.py

Pierre-Sassoulas · 2019-01-08T21:22:46Z

Turn out the error came from the very first commit. I thought importing from a package init.py's file is not the same as importing from a file named like the package. I might have to put back everything into the same file because I have no clue how to fix those 3 errors.

Pierre-Sassoulas · 2019-01-08T22:40:45Z

@PCManticore, do you have any idea why this commit break 3 functional tests ? I sure don't, and I opened this stackoverflow question regarding the problem.

PCManticore · 2019-01-13T14:28:33Z

@Pierre-Sassoulas It's definitely not related to your changes. I recently reverted some changes in astroid which affects pylint as well. Feel free to ignore them until we sort this out.

Pierre-Sassoulas · 2019-01-13T20:27:57Z

@PCManticore, thank you for taking the time to answer. I should rebase less or test the master branch more :)

So I guess this pull request is ready, it fails only for the same tests than the master branch.

PCManticore · 2019-01-30T18:19:46Z

I've been intending to review this PR, but it's so big that I can't find a big enough chunk of time to do so. I'll try to go over it over the weekend.

Pierre-Sassoulas · 2019-01-30T19:19:22Z

@PCManticore sorry the git diff is very big but it's basically just :

copy pasting code from pylint/utils.py to pylint/utils/class_name.py or pylint/utils/utils.py.
changing import so it still works like before after the copy paste
and then sorting those imports with isort (I could make a PR to include an isort configuration compatible with black in the pre-commit config, in a different PR if you want.).

I hope this help you review this pull request more easily.

brycepg · 2019-03-07T23:54:49Z

Since this PR has been here for so long and all the tests pass I'd be ok with this being merged as is but I'd like @PCManticore to do the merging. I think this PR makes utils on the whole easier to read.

Some post-merge thoughts:

WarningScope probably doesn't need to be in it's own file
_rest_format_section could be in message_handler_mix_in.py
If we're not careful we could end up with cyclical imports

Other non refactor thoughts:

Why is Message in utils? It would make sense to be in something like pylint/message.py since it's such a central object.
wow the way PyLinter uses these mixins is weird

Pierre-Sassoulas · 2019-03-08T06:08:27Z

Right, @brycepg I had the same idea than you about messages related classes. There is a lot about messages in utils. But, it could (should?) be in a message package. Yesterday I was about to create a message package little by little in order to make this giant PR in small easier step but I did not find the time.

I can take post-merge thought into account pre-merge. :) Let me know what you think.

Btw we DO end up with cyclical import as soon as we want to put checker related function back into the checker package.

(I also agree that the mixins are hard to understand).

Following review see : pylint-dev#2654 (comment)

Pierre-Sassoulas · 2019-03-08T07:01:41Z

My bad, WarningScope was in its own file to prevent circular import. It's longer to fix the circular import than to just let it be as it is, but I could do it later if you think it's important.

Following review here : pylint-dev#2654

Following review see : pylint-dev#2654 (comment)

Pierre-Sassoulas · 2019-03-08T07:57:37Z

I made a message package with message related class and constant but there was some conflict during rebase of the most recent change in the master branch. I won't have time to fix it today.

PCManticore

Hey @Pierre-Sassoulas Thanks for the ping. Just gave it a quick look and looks pretty amazing, thank you so much. Left a bunch of small comments, do you plan to add anything else or is the PR ready to go in? If so, we can merge it as soon as possible.

PCManticore · 2019-03-08T16:35:18Z

pylint/utils/normalize_text.py

+import textwrap
+
+
+def normalize_text(text, line_len=80, indent=""):


Seems odd to have a module just for this function, is there a way to move it somewhere else?

It's used in a lot of places and I think there is a circular import problem (Like for WarningScope). It's probably possible to fix, but that require some design.

pylint/utils/pylint_ast_walker.py

pylint/utils/utils.py

PCManticore · 2019-03-08T16:36:56Z

pylint/utils/utils.py

@@ -0,0 +1,376 @@
+# -*- coding: utf-8 -*-


Feels odd to have an utils.py inside an utils package, but I don't have a better suggestion.

This permit to change the from pylint import utils to from pylint.utils import utils, AstWalker, ... and the class can be imported one by one while still using the utils.function_name() for functions. It makes some change elsewhere less painful. Ideally every utils function would be in a semantically significant module or package but there is a lot of those utility functions...

There is a lot of Message related class in Utils this warrant the creation of a new package. See also review for burst utils.py into a package here: pylint-dev#2654 (comment)

Taken from unittest_lint.py

For obvious reasons. See review here : pylint-dev#2654 (comment)

Pierre-Sassoulas · 2019-03-08T19:51:24Z

@PCManticore I made the requested change for the discussion I resolved, and I pushed a rebased version for review. There is two unresolved discussions (about the utils.utils.py file and normalize_text.py). I can implements the solution if you have an idea to make those better.

Following review see : #2654 (comment)

There is a lot of Message related class in Utils this warrant the creation of a new package. See also review for burst utils.py into a package here: #2654 (comment)

For obvious reasons. See review here : #2654 (comment)

PCManticore · 2019-03-09T10:10:05Z

@Pierre-Sassoulas Thank you so much for this cleanup! I don't have any strong opinions about utils.utils and normalize_text but those can be handled in a separate PR, no need to keep this one waiting for that.

Also fixed spelling in C0112 to C0116 messages following the review of pull-request pylint-dev#2036 by Ashley Whetter. See also pylint-dev#2075, pylint-dev#2077, pylint-dev#2262, pylint-dev#2654, pylint-dev#2805, pylint-dev#2809, pylint-dev#2844, pylint-dev#2992 and pylint-dev#3013 for full historical context.

Also fixed spelling in C0112 to C0116 messages following the review of pull-request #2036 by Ashley Whetter. See also #2075, #2077, #2262, #2654, #2805, #2809, #2844, #2992 and #3013 for full historical context.

this is due to pylint-dev/pylint#2654 moving symbols into different places.

Pierre-Sassoulas mentioned this pull request Dec 18, 2018

Refactor of MessageStore for multiple old message #2262

Merged

Pierre-Sassoulas commented Dec 19, 2018

View reviewed changes

pylint/utils/utils.py Outdated Show resolved Hide resolved

Pierre-Sassoulas force-pushed the utils_refactor branch from de11d8a to b330c66 Compare December 25, 2018 13:23

Pierre-Sassoulas force-pushed the utils_refactor branch 3 times, most recently from 6601f05 to f303185 Compare January 8, 2019 21:19

Pierre-Sassoulas force-pushed the utils_refactor branch from f303185 to 314aece Compare January 8, 2019 22:24

Pierre-Sassoulas force-pushed the utils_refactor branch from 314aece to 1760d34 Compare January 13, 2019 20:23

Pierre-Sassoulas force-pushed the utils_refactor branch 2 times, most recently from c5456c7 to 16c776c Compare January 27, 2019 21:22

Pierre-Sassoulas force-pushed the utils_refactor branch from 16c776c to 757b2a5 Compare February 23, 2019 14:22

Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Mar 8, 2019

Refactor - WarningScope doesn't need to be in its own file

eedeb55

Following review see : pylint-dev#2654 (comment)

Pierre-Sassoulas force-pushed the utils_refactor branch from eedeb55 to bf7659f Compare March 8, 2019 06:58

Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Mar 8, 2019

Refactor - Move _rest_format_section in message_handler_mix_in.py

bf7659f

Following review see : pylint-dev#2654 (comment)

Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Mar 8, 2019

Refactor - Create the pyling.message package

60e197b

Following review here : pylint-dev#2654

Pierre-Sassoulas added 5 commits March 8, 2019 08:51

Chore - Move utils.py into its own package

91c917d

Chore - Move content from __init__.py to utils.py

bb1c2c9

Refactor - Create a file for the MessageStore class

ef9d73d

Refactor - Create a file for each class in utils

3444f57

Fix - C0412: Imports from package os are not grouped

ca2ba2e

Refactor - Move _rest_format_section in message_handler_mix_in.py

3de592c

Following review see : pylint-dev#2654 (comment)

Pierre-Sassoulas force-pushed the utils_refactor branch from bf7659f to 3de592c Compare March 8, 2019 07:54

PCManticore approved these changes Mar 8, 2019

View reviewed changes

Pierre-Sassoulas added 7 commits March 8, 2019 19:49

Refactor - Create a pylint.message package

8aedc31

There is a lot of Message related class in Utils this warrant the creation of a new package. See also review for burst utils.py into a package here: pylint-dev#2654 (comment)

Refactor - Create a unittest file for the pylint.message package

49450ca

Refactor - Move test_create_invalid_message_type in pylint.message test

f526da9

Refactor - Move test_register_error in pylint.message test

7205735

Refactor - Create a unittest file for MessageStore

26d693a

Taken from unittest_lint.py

Refactor - Rename PylintASTWalker to ASTWalker

93a0a36

For obvious reasons. See review here : pylint-dev#2654 (comment)

Refactor - Rename _ini_format_section directly to format_section

cf7a3af

Pierre-Sassoulas changed the title ~~Refactor - Burst utils.py into a "utils" package~~ Refactor - Burst utils.py into a "utils" and "message" package Mar 8, 2019

PCManticore approved these changes Mar 9, 2019

View reviewed changes

PCManticore merged commit 5073ebd into pylint-dev:master Mar 9, 2019

PCManticore pushed a commit that referenced this pull request Mar 9, 2019

Refactor - Move _rest_format_section in message_handler_mix_in.py

b1780d1

Following review see : #2654 (comment)

PCManticore pushed a commit that referenced this pull request Mar 9, 2019

Refactor - Create a pylint.message package

2b50749

There is a lot of Message related class in Utils this warrant the creation of a new package. See also review for burst utils.py into a package here: #2654 (comment)

PCManticore pushed a commit that referenced this pull request Mar 9, 2019

Refactor - Rename PylintASTWalker to ASTWalker

b2a77e4

For obvious reasons. See review here : #2654 (comment)

Pierre-Sassoulas deleted the utils_refactor branch March 9, 2019 11:07

Pierre-Sassoulas mentioned this pull request Mar 9, 2019

Refactor to remove circular import in utils/reporter/checker #2809

Merged

Pierre-Sassoulas restored the utils_refactor branch March 21, 2019 19:01

Pierre-Sassoulas deleted the utils_refactor branch March 21, 2019 19:01

Anthchirp added a commit to Anthchirp/pylint-django-feedstock that referenced this pull request Nov 2, 2020

pylint >=2.4.0 is currently not supported

6418223

this is due to pylint-dev/pylint#2654 moving symbols into different places.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor - Burst utils.py into a "utils" and "message" package #2654

Refactor - Burst utils.py into a "utils" and "message" package #2654

Pierre-Sassoulas commented Dec 18, 2018 •

edited

coveralls commented Dec 18, 2018 •

edited

Pierre-Sassoulas commented Jan 8, 2019 •

edited

Pierre-Sassoulas commented Jan 8, 2019

PCManticore commented Jan 13, 2019

Pierre-Sassoulas commented Jan 13, 2019

PCManticore commented Jan 30, 2019

Pierre-Sassoulas commented Jan 30, 2019 •

edited

brycepg commented Mar 7, 2019 •

edited

Pierre-Sassoulas commented Mar 8, 2019

Pierre-Sassoulas commented Mar 8, 2019

Pierre-Sassoulas commented Mar 8, 2019

PCManticore left a comment

PCManticore Mar 8, 2019

Pierre-Sassoulas Mar 8, 2019

PCManticore Mar 8, 2019

Pierre-Sassoulas Mar 8, 2019

Pierre-Sassoulas commented Mar 8, 2019

PCManticore commented Mar 9, 2019

		import textwrap


		def normalize_text(text, line_len=80, indent=""):

Refactor - Burst utils.py into a "utils" and "message" package #2654

Refactor - Burst utils.py into a "utils" and "message" package #2654

Conversation

Pierre-Sassoulas commented Dec 18, 2018 • edited

coveralls commented Dec 18, 2018 • edited

Pierre-Sassoulas commented Jan 8, 2019 • edited

Pierre-Sassoulas commented Jan 8, 2019

PCManticore commented Jan 13, 2019

Pierre-Sassoulas commented Jan 13, 2019

PCManticore commented Jan 30, 2019

Pierre-Sassoulas commented Jan 30, 2019 • edited

brycepg commented Mar 7, 2019 • edited

Pierre-Sassoulas commented Mar 8, 2019

Pierre-Sassoulas commented Mar 8, 2019

Pierre-Sassoulas commented Mar 8, 2019

PCManticore left a comment

Choose a reason for hiding this comment

PCManticore Mar 8, 2019

Choose a reason for hiding this comment

Pierre-Sassoulas Mar 8, 2019

Choose a reason for hiding this comment

PCManticore Mar 8, 2019

Choose a reason for hiding this comment

Pierre-Sassoulas Mar 8, 2019

Choose a reason for hiding this comment

Pierre-Sassoulas commented Mar 8, 2019

PCManticore commented Mar 9, 2019

Pierre-Sassoulas commented Dec 18, 2018 •

edited

coveralls commented Dec 18, 2018 •

edited

Pierre-Sassoulas commented Jan 8, 2019 •

edited

Pierre-Sassoulas commented Jan 30, 2019 •

edited

brycepg commented Mar 7, 2019 •

edited