Skip to content

Refactor of MessageStore for multiple old message #2262

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
Merged
PCManticore merged 5 commits into from
Dec 18, 2018
Merged

Refactor of MessageStore for multiple old message #2262

PCManticore merged 5 commits into from
Dec 18, 2018

Conversation

@Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented Jul 7, 2018

Work in progress of the refactor for #2036

In order to see what still need to be done.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the msgstore_refactor branch 2 times, most recently from a413f99 to f5b17ac Compare July 7, 2018 14:05
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Jul 7, 2018

Sorry this is super messy but starting by really changing MessageStore was not a reasonable way to do it (way too complex). So I just changed line 888 in python/utils.py in order to return a list and now I'm trying to pass existing tests. I'm afraid there will be a lot of changes to do. For exemple somewhere we use get_message_definition to return a symbol. But we now need to return a list of symbol, so the refactor will spread elsewhere... Also for some strange reason I can't test in my setup, I get "AttributeError: 'Assign' object has no attribute 'type_annotation'" everywhere.

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Jul 7, 2018

@PCManticore this is not ready for review yet, I'm using the travis to circumvent my local bug with the tests :)

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the msgstore_refactor branch 2 times, most recently from ddc2d02 to b457fd0 Compare July 9, 2018 12:56
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Jul 9, 2018

@PCManticore, I have the following error locally but the same code does not have the problem on the Pylint Travis... It prevent me from using pbd to debug. Pushing to Travis everytime is not very efficient. Any idea of the reason I get that ?

  File "m:\pylint\.tox\pylint\lib\site-packages\pylint\checkers\variables.py", line 1380, in _store_type_annotation_names
    type_annotation = node.type_annotation
AttributeError: 'Assign' object has no attribute 'type_annotation'

I saw this post, is it a known current problem with cygwin/mingw ?

@PCManticore
Copy link
Contributor

PCManticore commented Jul 9, 2018

@Pierre-Sassoulas You need the latest astroid for getting read of that error, I'd suggest rebuilding the tox environment with tox --recreate, this should bring the latest astroid.

@PCManticore PCManticore added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Jul 23, 2018
@PCManticore
Copy link
Contributor

PCManticore commented Jul 23, 2018

@Pierre-Sassoulas Going to review this once the dust settles for the 2.0 release, so most likely next week. Sorry for the delay!

PCManticore
PCManticore approved these changes Aug 24, 2018
View reviewed changes
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.

This looks 💯 @Pierre-Sassoulas ! Seems the CI is failing but once it's green, we can ship it. I'll take a look at your missing-docstring PR this weekend.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the msgstore_refactor branch 4 times, most recently from c47d6e0 to d9f2c87 Compare November 1, 2018 22:34
@coveralls
Copy link

coveralls commented Nov 1, 2018

Coverage Status

Coverage decreased (-0.1%) to 89.557% when pulling d9f2c87 on Pierre-Sassoulas:msgstore_refactor into 9734bae on PyCQA:master.

1 similar comment
@coveralls
Copy link

coveralls commented Nov 1, 2018

Coverage Status

Coverage decreased (-0.1%) to 89.557% when pulling d9f2c87 on Pierre-Sassoulas:msgstore_refactor into 9734bae on PyCQA:master.

@coveralls
Copy link

coveralls commented Nov 1, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 89.722% when pulling ef2bd70 on Pierre-Sassoulas:msgstore_refactor into 1c057b0 on PyCQA:master.

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Nov 1, 2018

Hi @PCManticore.

Thanks to the national holidays the tests for this are now passing... I modified some tests, and I'm surprised there is so little changes. If I were you I would check with care what was changed before merging. At least what was changed in the tests.

This is a first step towards being able to have multiple values for a message in MessageStore. Before we returned a message Definition. In this PR we always have a list of only one value, next step will be to actually change the MessageStore to have a list of multiple MessageDefinition for an old name. As I said this a big refactor and it's easier done with small incremental steps.

Slightly related my astroid was deprecated again. It's hard to see, especially when you're working on something that is supposed to break the tests. It took me some time to think of testing that the master branch tests were still passing. Maybe you should add the "tox --recreate" advice in the doc ?

Good job on adding black and a pre-commit hook !

Regards,

@PCManticore PCManticore mentioned this pull request Nov 6, 2018
Closed
@PCManticore
Copy link
Contributor

PCManticore commented Nov 6, 2018

Hey @Pierre-Sassoulas Great stuff, thank you coming back at this! I'll do a final review and merge this weekend so that could unblock you for your other PR.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the msgstore_refactor branch 2 times, most recently from b7a8b57 to f0e8a77 Compare December 9, 2018 16:53
@PCManticore
Copy link
Contributor

PCManticore commented Dec 11, 2018

@Pierre-Sassoulas There's something wrong with this PR, it has quite a lot of changes. :) I'd like to merge it this week, so if you have some time to revert the inadvertent changes, that would be amazing.

@Pierre-Sassoulas @psass-antidot
Refactor - get_message_definition returning a list
a48a7b3 
Of MessageDefinition instead of a MessageDefinition.
@Pierre-Sassoulas @psass-antidot
Refactor - Consistent function and variable name
fffaa8d 
We return a list of message definitions so we must rename
get_message_definition to get_message_definitions. Resulting variables
are now named message_definitions most of the time in order to improve
readability. (It was often named "msgs" or worst "symbol")
@Pierre-Sassoulas @psass-antidot
Refactor - get_messages -> get_messages_from_checker
33292b1
@Pierre-Sassoulas @psass-antidot
Refactor - Made the error message predictable
3d3082a 
When refactoring MsgStore we don't want the test to break if two
conflicting symbol or msgid are inverted. So we sort them, in order
for the error message to stay stable.
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Dec 11, 2018

@PCManticore sorry, I pushed by mistakes the change I did for the next step, should be ok now.

@PCManticore PCManticore merged commit 75cecdb into pylint-dev:master Dec 18, 2018
@PCManticore
Copy link
Contributor

PCManticore commented Dec 18, 2018

@Pierre-Sassoulas Thank you for waiting! This is merged now, let me know if you need anything for #2036

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Dec 18, 2018

@PCManticore thank you for merging this first step. As you said earlier there is a lot of change in my current branch. Should I try to make multiple PR or should I put everything in #2036 ? There is a break up of utils.py into a package for example, and a renaming of 'MessageDefinition' to 'Message'.

@PCManticore
Copy link
Contributor

PCManticore commented Dec 18, 2018

@Pierre-Sassoulas Separate PRs could be better, since we can merge those faster than a big chunk.

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Dec 18, 2018

Sorry for the delay, I had a hard time separating the changes. Here's the first one, the refactor of utils.py : #2654

Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Aug 4, 2019
@Pierre-Sassoulas
[missing-docstring] Break down into class, function, and module
648b7d4 
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
@Pierre-Sassoulas
[missing-docstring] Break down into class, function, and module
efc49c7 
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
@Pierre-Sassoulas
[missing-docstring] Break down into class, function, and module
c66359b 
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
@Pierre-Sassoulas
[missing-docstring] Break down into class, function, and module
c8b0fc3 
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
@Pierre-Sassoulas @PCManticore
[missing-docstring] Break down into class, function, and module
65fb74d 
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 Pierre-Sassoulas removed the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PCManticore PCManticore PCManticore approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Pierre-Sassoulas @PCManticore @coveralls