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.

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

Refactor to remove circular import linked to build_message_definition #2844

Conversation

Pierre-Sassoulas
Copy link
Member

Work in progress for removing circular import with build_message_definition. Following the refactor to use checker inside the documentation function, 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.

Description

  • build_message_definition is used in pylint.checker.base_checker and also in message.MessageHandlerMixIn.print_full_documentation.
  • The idea is to put this function in the checker packages, to break the cycle
  • So we need to use a Checker instance in print_full_documentation
  • Doing this refactoring makes MyPy realize that MessageHandlerMixIn do not have a get_checkers() function
    So there is probably a lot of thing to change to fix this.

Type of Changes

| ✓ | 🔨 Refactoring |

Related Issue

Based on #2809 but this is not mergeable yet so creating another PR.

Copy link
Contributor

@PCManticore PCManticore left a comment

Choose a reason for hiding this comment

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

Hey @Pierre-Sassoulas Sorry for the delayed review! This looks great, but there are some minor changes we need before getting this in, the major one being not using variable type assignments given that we still support 3.4 and 3.5.

pylint/message/message_id.py Outdated Show resolved Hide resolved
pylint/message/message_handler_mix_in.py Outdated Show resolved Hide resolved
pylint/message/message_handler_mix_in.py Outdated Show resolved Hide resolved
pylint/checkers/base_checker.py Outdated Show resolved Hide resolved
pylint/checkers/base_checker.py Outdated Show resolved Hide resolved
pylint/checkers/base_checker.py Show resolved Hide resolved
@Pierre-Sassoulas
Copy link
Member Author

Hi, @PCManticore thank you for the review. I took what you said into account but I'm not pushing the change yet because the BaseChecker.__doc__ function is not used/working as of now. What is done in MessageHandlerMixIn for printing documentation is really hard to understand and refactor. The printing and string creation was mixed and there are inputs informations that are different when it's called from doc/exts/pylint_extensions.py , than directly from MessageHandlerMixIn.get_full_documentation() so the information contained in checker are not sufficient for all cases. I'll ping you once the tests are passing.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the build-message-definition-in-checker branch from 56243ee to 3d1de86 Compare June 10, 2019 07:30
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 10, 2019
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 10, 2019
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 10, 2019
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 10, 2019
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 89.868% when pulling 3d1de86 on Pierre-Sassoulas:build-message-definition-in-checker into e3d4e80 on PyCQA:master.

@coveralls
Copy link

coveralls commented Jun 10, 2019

Coverage Status

Coverage decreased (-0.06%) to 89.868% when pulling 0918e02 on Pierre-Sassoulas:build-message-definition-in-checker into ee228dd on PyCQA:master.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the build-message-definition-in-checker branch from 3d1de86 to d4d3c92 Compare June 10, 2019 08:02
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 10, 2019
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 10, 2019
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 10, 2019
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the build-message-definition-in-checker branch from 0fa78e9 to d8b7211 Compare June 10, 2019 08:23
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Jun 10, 2019

Hi @PCManticore. I took some time to fix this, there was a lot of disentanglement of print and output producing with implicit line feed when you use print to take into account. Sadly I could not keep the __doc__ function in BaseChecker, because if we were using a docstring in a checker it was overriding the BaseChecker.__doc__ function. I added a full exact output test during the development so we should produce exactly the same output after this refactor. We can't keep this test because the output is very long and change when checkers change. I could add some more string in the regex test though. I think there is a pre-existing problem with python 3.8-dev not caused by my change. I don't understand the problem with appveyor for python 3.4. Maybe it's preexisting too?

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the build-message-definition-in-checker branch 2 times, most recently from 3c355df to 1f5ec63 Compare June 10, 2019 10:11
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 10, 2019
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 10, 2019
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 10, 2019
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the build-message-definition-in-checker branch from 92216e2 to 5ad5871 Compare June 12, 2019 19:37
@PCManticore
Copy link
Contributor

Hey @Pierre-Sassoulas This looks great, thank you for the hard work! The Python 3.4 failure is preexisting, as well as the 3.8 one. But I noticed that docs and mypy steps failed for you, can you fix those before getting this in?

@Pierre-Sassoulas
Copy link
Member Author

Sure. I'll also rebase and fix the merging conflicts that appeared.

Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 20, 2019
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Jun 20, 2019
Because we want to make it a public function.
We want to use them in Checker too.
Mypy requirements and python 3.4 seem incompatible.
I used the old name but there was probably a typo in it, as the format is
called rst.
This make the understanding of the function easier.
BaseChecker and MessageHandlerMixIn can be the same instance.
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the build-message-definition-in-checker branch from 5ad5871 to 0918e02 Compare June 20, 2019 11:19
@Pierre-Sassoulas
Copy link
Member Author

@PCManticore I fixed the merge conflict, mypy, and docs. I have no idea about the error by appveyor though, is it a completely different test than tox? How can I launch it locally?

@PCManticore
Copy link
Contributor

Thanks for fixing this! Don't worry about appveyor, it's broken in master as well. In fact it is a form of TravisCI but for Windows, it's not possible to test it locally unless you have a Windows VM or a Windows OS.

@PCManticore PCManticore merged commit 298a22d into pylint-dev:master Jun 20, 2019
PCManticore pushed a commit that referenced this pull request Jun 20, 2019
PCManticore pushed a commit that referenced this pull request Jun 20, 2019
PCManticore pushed a commit that referenced this pull request Jun 20, 2019
@Pierre-Sassoulas Pierre-Sassoulas deleted the build-message-definition-in-checker branch June 20, 2019 18:40
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Aug 4, 2019
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.
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Aug 19, 2019
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.
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Sep 10, 2019
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.
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Sep 10, 2019
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.
PCManticore pushed a commit that referenced this pull request Sep 10, 2019
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants