Refactor of MessagesStore #2075
Conversation
Create a function to get message definitions list from a checker. Create a function for checking checker consistency Create a function in order to register a MessageDefinition Corrected following the review of pull-request pylint-dev#2036 by Ashley Whetter and PCManticore.
Use of f-strings in Python 3. Updated the docstrings with spinx syntax. Rename variable to be clearer and use @staticmethoc when possible. Following review of pylint-dev#2036
|
Apparently the f-string are failing for python 3.4 and 3.5. |
|
@Pierre-Sassoulas I'm sorry, I completely forgot about the fact that we still support 3.4 and 3.5. While we could drop the support for these and start from 3.6+ only, it's a bit too much for this release. So we can't use f-strings for now, sorry again for leading you astray! |
|
No problem, the way I fixed it should hopefully make it easier to go back to fstring again at a later date. |
There was a problem hiding this comment.
Looks great @Pierre-Sassoulas ! Left a couple of comments for you to address before getting this in
pylint/test/unittest_lint.py
Outdated
| store.add_renamed_message( | ||
| 'W1337', 'msg-symbol', 'duplicate-keyword-arg') | ||
| assert str(cm.value) == "Message symbol 'msg-symbol' is already defined" | ||
| expected = "Message id 'W1234' cannot have both 'msg-symbol' and \ |
There was a problem hiding this comment.
Please don't use \ as a line continuation. Do an implicit string join instead with parens:
("Message ..."
"and ...")
pylint/utils.py
Outdated
|
|
||
| def get_msg_display_string(self, msgid): | ||
| """Generates a user-consumable representation of a message. | ||
| """ Generates a user-consumable representation of a message. |
There was a problem hiding this comment.
Why the extraneous space?
pylint/utils.py
Outdated
|
|
||
| @staticmethod | ||
| def _raise_duplicate_msg_id(symbol, msgid, other_msgid): | ||
| """ Raise an error when a msgid is duplicated. |
There was a problem hiding this comment.
There's an extraneous first space for each docstring.
pylint/utils.py
Outdated
| :param str symbol: The symbol corresponding to the msgids | ||
| :param str msgid: Offending msgid | ||
| :param str other_msgid: Other offending msgid | ||
| :raises InvalidMessageError: when a msgid is duplicated. """ |
There was a problem hiding this comment.
Please add the triple quotes on the next line.
pylint/utils.py
Outdated
| """ Register messages from a checker. | ||
|
|
||
| :param BaseChecker checker: """ | ||
| messages = MessagesStore.get_checker_message_definitions(checker) |
There was a problem hiding this comment.
You can access these staticmethods using self instead.
pylint/utils.py
Outdated
| if msg.symbol == symbol: | ||
| other_msgid = msg.msgid | ||
| else: | ||
| for old_msgid, old_symbol in msg.old_names: |
There was a problem hiding this comment.
If we'll ever drop the old_names from the codebase the code at line 794 will start to fail since other_msgid is no longer defined, so make sure to define it somewhere as None at least.
There was a problem hiding this comment.
Like in a lot of other places I copypasted the existing code and just created function with identical behavior. I'll just add the initialization to None, because this function will probably be deleted with #2036.
pylint/utils.py
Outdated
|
|
||
| def _check_symbol(self, msgid, symbol): | ||
| """ Check that a symbol is not already used. """ | ||
| if symbol in self._messages: |
There was a problem hiding this comment.
You can refactor the following if to:
other_msgid = self._messages.get(symbol)
if other_msgid:
Now raise it
pylint/utils.py
Outdated
| if symbol in self._messages: | ||
| other_msgid = self._messages[symbol].msgid | ||
| MessagesStore._raise_duplicate_msg_id(symbol, msgid, other_msgid) | ||
| if symbol in self._alternative_names: |
There was a problem hiding this comment.
Ditto regarding fetching the symbol and checking that the value is not None.
pylint/utils.py
Outdated
| MessagesStore._raise_duplicate_msg_id(symbol, msgid, other_msgid) | ||
|
|
||
| def _check_msgid(self, msgid, symbol): | ||
| for message_symbol in self._messages: |
There was a problem hiding this comment.
Why don't you iterate over .items() instead?
There was a problem hiding this comment.
I copy pasted the old code :)
pylint/utils.py
Outdated
| old_symbolic_name = alternate_symbol | ||
| if symbol not in (alternative.symbol, old_symbolic_name): | ||
| if msgid == old_symbolic_id: | ||
| MessagesStore._raise_duplicate_symbol(msgid, symbol, old_symbolic_name) |
There was a problem hiding this comment.
Ditto regarding staticmethods and accessing them with self.
And adapt line lenght in order to not add convention message.
Pierre-Sassoulas
force-pushed
the
refactor_message_store
branch
from
e1c1f65
to
eb0868f
Compare
Pierre-Sassoulas
force-pushed
the
refactor_message_store
branch
from
38ee76c
to
9b264e8
Compare
|
I think I adressed everything, but feel free to check. I will make another PR for #2071 once this is merged to master. |
|
Thank you @Pierre-Sassoulas ! Looks great! Looking forward to merge the missing-docstring PR as well |
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 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 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 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.
I've refactored quite a lot during the implementation of #2036, this seem to be independant of the functional change required. This pull request add readability changes and better error messages for duplicated or non existing message, following the review by @AWhetter and @PCManticore.