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

Pierre-Sassoulas · 2019-03-09T21:58:08Z

Description

The idea of this PR is to break dependency cycle by having every constant in pylint/constant.py and then every package depending on pylint/utils.

In order to do this we created a constant.py file, and moved some class from utils to other package (utils.ReportsHandlerMixIn => reporter.ReportsHandlerMixIn) so that nothing in pylint.utils depends on pylint.checker or pylint.reporter. We also moved some function to where they made more sense. For example functions handling Checker in MessageStore were moved to the base checker class so they can be called from any instance of checker. Or unit test for checker.utils were moved from unit test for utils to unit test for checker.utils.

This permit to remove the two files we had to create in pylint.utils with only the small normalized_text and WarningScope inside it, and simplify the code.

Type of Changes

| ✓ | 🔨 Refactoring |

Related Issue

Fix problem discussed in #2654 for normalized_text and WarningScope.

coveralls · 2019-03-09T22:22:43Z

Coverage decreased (-0.05%) to 89.653% when pulling be9f0eb on Pierre-Sassoulas:fix-circular-import-in-utils into 75a0479 on PyCQA:master.

Pierre-Sassoulas · 2019-03-10T21:53:19Z

@PCManticore, this is a follow up for #2654 to removes circular imports in utils/checker/reporter. It's ready for review :)

pylint/utils/utils.py

PCManticore

@Pierre-Sassoulas I have this on my todo list for reviews, I hope I can get to it this week.

.gitignore

pylint/checkers/base.py

Pierre-Sassoulas · 2019-03-19T10:13:52Z

@PCManticore, thank you for starting a review, but please do not plan time for this PR this week : I'll focus on #2805 in order to merge it first. It will make the review of this one a lot easier afterward. (There is the same kind of problems, I'll fix them once and rebase.)

Pierre-Sassoulas · 2019-03-24T17:53:52Z

Hey @PCManticore, I looked at the reason of the error for the previous push. It seems that a commit makes MyPy suddenly raise a "MessagesHandlerMixIn" has no attribute "get_checkers"; that it was'nt raising before. MessagesHandlerMixIn never had a get_checkers function, but MyPy did not detect it before. the commit was just separating information retrieval from string creation in a MessagesHandlerMixIn function. I don't want to refactor the multi-inheritance and the HandlerMixIn, so I removed the commits. As a result the build_message_definition function cannot be moved to Checker and will remain alone in a file.

So this PR do not solve every circular import but solving this would take too much time.

PCManticore

@Pierre-Sassoulas This looks great, thank you for tackling it! The direction is great and left a couple of comments to be addressed before merging this in.

pylint/checkers/base_checker.py

PCManticore · 2019-03-28T09:29:02Z

pylint/message/__init__.py

-    MSG_TYPES_LONG,
-    MSG_TYPES_STATUS,
-)
+from pylint.message.build_message_definition import build_message_definition


❓ I didn't notice this last time, is it possible to move build_message_definition into message_definition itself so we can squash that module with a single exported function?

I tried but there definitely is a circular import here :

py36/lib/python3.6/site-packages/pylint/checkers/__init__.py:43: in <module> from pylint.checkers.base_checker import BaseChecker, BaseTokenChecker py36/lib/python3.6/site-packages/pylint/checkers/base_checker.py:19: in <module> from pylint.message.message_definition import build_message_definition py36/lib/python3.6/site-packages/pylint/message/__init__.py:43: in <module> from pylint.message.message_handler_mix_in import MessagesHandlerMixIn py36/lib/python3.6/site-packages/pylint/message/message_handler_mix_in.py:24: in <module> from pylint.message.message_definition import build_message_definition py36/lib/python3.6/site-packages/pylint/message/message_definition.py:12: in <module> from pylint.message.message_definition import MessageDefinition E ImportError: cannot import name 'MessageDefinition'

I wanted to move the message building in BaseChecker to fix this, but the refactor is humongous see : #2844

Oh gotcha, thanks for the extra explanation.

pylint/utils/normalize_text.py

pylint/utils/utils.py

pylint/checkers/base_checker.py

Some constants were package internal but were used by multiple packages. This created circular dependencies. By creating a file for constants we make sure this does not happen because we won't import everything important in this file and every thing else can depend on it.

ReportsHandlerMixIn was importing Nodes from reporter and is probably more suited for the reporter package anyway.

Now that there is no more circular import we can do that.

This permit to have less cross dependency as the utils package does not depend on anything. The checker package still depends on reporter. Also moved classes from __init__ to their own file in reporter.

It turns out there is not that much utils unittests.

MsgStore.get_messages_from_checker => Checker.messages MsgStore.check_checker_consistency => Checker.check_consistency Probably makes more sense this way.

It was 'hidden' in checkers.__init__.py

Co-Authored-By: Pierre-Sassoulas <[email protected]>

There might be code that is used only in test but it's harder to detect.

Pierre-Sassoulas · 2019-03-28T22:00:54Z

There is a test failing in the doc, but I don't think ide-integration.rst is generated, so I think the problem might come from the master branch. What do you think @PCManticore ?

PCManticore · 2019-03-29T08:34:33Z

@Pierre-Sassoulas This is definitely coming from master. We weren't pinning sphinx in tox and the latest one has issues with that code block. This is now fixed in master.

PCManticore · 2019-03-29T08:37:01Z

@Pierre-Sassoulas Did a run with this PR locally and it passes the tests. Let's merge it as is since it's good to go.

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.

Pierre-Sassoulas force-pushed the fix-circular-import-in-utils branch 4 times, most recently from 07eab4f to 491c0a2 Compare March 10, 2019 21:21

Pierre-Sassoulas commented Mar 11, 2019

View reviewed changes

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

PCManticore reviewed Mar 19, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

pylint/checkers/base.py Outdated Show resolved Hide resolved

Pierre-Sassoulas force-pushed the fix-circular-import-in-utils branch 8 times, most recently from 1a2d8f4 to be9f0eb Compare March 24, 2019 17:32

Pierre-Sassoulas mentioned this pull request Mar 28, 2019

Breaking down C0111(Missing Docstring) into multiple "Convention" messages #1164

Closed

PCManticore reviewed Mar 28, 2019

View reviewed changes

Pierre-Sassoulas and others added 9 commits March 28, 2019 13:41

Refactor - Remove circular import between utils and reporter

d621251

ReportsHandlerMixIn was importing Nodes from reporter and is probably more suited for the reporter package anyway.

Refactor - Merge normalize text in utils.py

a017319

Now that there is no more circular import we can do that.

Refactor - Move refactor.utils function to the utils package

8fcf693

This permit to have less cross dependency as the utils package does not depend on anything. The checker package still depends on reporter. Also moved classes from __init__ to their own file in reporter.

Refactor - Create a new file for ASTWalker unittest

f3dabc1

Fix - Move checker.utils tests into their dedicated unittest

ce0fb00

It turns out there is not that much utils unittests.

Refactor - Move function for checker in Checker

ab463d0

MsgStore.get_messages_from_checker => Checker.messages MsgStore.check_checker_consistency => Checker.check_consistency Probably makes more sense this way.

Refactor - Create file for BaseChecker

1b9e006

It was 'hidden' in checkers.__init__.py

Style - Use a single list comprehension instead of a for loop

bba5eae

Co-Authored-By: Pierre-Sassoulas <[email protected]>

Pierre-Sassoulas force-pushed the fix-circular-import-in-utils branch from be9f0eb to 1b9e006 Compare March 28, 2019 20:01

Pierre-Sassoulas mentioned this pull request Mar 28, 2019

Refactor to remove circular import linked to build_message_definition #2844

Merged

Refactor - Remove unused and untested code in utils

6e6e655

There might be code that is used only in test but it's harder to detect.

Pierre-Sassoulas force-pushed the fix-circular-import-in-utils branch from 74e2dba to fb2edfa Compare March 28, 2019 21:06

Fix - Remove the redefined build-in instead of using a pragma

400a060

Pierre-Sassoulas force-pushed the fix-circular-import-in-utils branch from fb2edfa to 400a060 Compare March 28, 2019 21:27

PCManticore merged commit a14fe17 into pylint-dev:master Mar 29, 2019

Pierre-Sassoulas deleted the fix-circular-import-in-utils branch March 29, 2019 12:01

Pierre-Sassoulas mentioned this pull request Feb 12, 2021

Use Squash Merge for merge requests #4084

Closed

Uh oh!

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

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

Uh oh!

Conversation

Pierre-Sassoulas commented Mar 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Changes

Related Issue

Uh oh!

coveralls commented Mar 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pierre-Sassoulas commented Mar 10, 2019

Uh oh!

Uh oh!

PCManticore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Pierre-Sassoulas commented Mar 19, 2019

Uh oh!

Pierre-Sassoulas commented Mar 24, 2019

Uh oh!

PCManticore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PCManticore Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

PCManticore Mar 29, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pierre-Sassoulas commented Mar 28, 2019

Uh oh!

PCManticore commented Mar 29, 2019

Uh oh!

PCManticore commented Mar 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Pierre-Sassoulas commented Mar 9, 2019 •

edited

Loading

coveralls commented Mar 9, 2019 •

edited

Loading