Merge MessagesHandlerMixIn into PyLinter
#5136
Conversation
DanielNoord
added
the
Maintenance
Pull Request Test Coverage Report for Build 1339491567
💛 - Coveralls |
| @@ -54,15 +54,21 @@ class ByIdManagedMessagesChecker(BaseChecker): | |||
| } | |||
| options = () | |||
|
|
|||
| def _clear_by_id_managed_msgs(self) -> None: | |||
There was a problem hiding this comment.
I found that these could be private methods here instead of classmethods on MessagesHandlerMixIn or PyLinter.
| try: | ||
| return [md.symbol for md in self.msgs_store.get_message_definitions(msgid)] | ||
| except exceptions.UnknownMessageError: | ||
| return [msgid] |
There was a problem hiding this comment.
This was changed to also return a list, to make the typing more consistent and easier.
| # msgid is a category? | ||
| category_id = msgid.upper() | ||
| if category_id not in MSG_TYPES: | ||
| category_id_formatted = MSG_TYPES_LONG.get(category_id) |
There was a problem hiding this comment.
I added the extra else to help mypy infer the correct type. category_id is now str, while category_id_formatted is Optional[str]
| @@ -540,6 +535,7 @@ def __init__(self, options=(), reporter=None, option_groups=(), pylintrc=None): | |||
| # Attributes related to message (state) handling | |||
| self.msg_status = 0 | |||
| self._msgs_state: Dict[str, bool] = {} | |||
| self._by_id_managed_msgs: List[ManagedMessage] = [] | |||
There was a problem hiding this comment.
This could also be a non private attribute. It was a private attribute in the previous code, but might need to be non-private now that it used in pylint/checkers/misc.py. Let me know if I should change this!
| self, | ||
| msgid: str, | ||
| scope: str = "package", | ||
| line: Optional[int] = None, |
There was a problem hiding this comment.
I removed the Union[bool, int, None] typing here. bool was incorrect and nonsensical.
| @@ -1514,3 +1510,152 @@ def add_ignored_message( | |||
| message_definition.msgid, | |||
| line, | |||
| ) | |||
|
|
|||
| # Setting the state (disabled/enabled) of messages and registering them | |||
There was a problem hiding this comment.
I also reordered some of the methods to (mostly) follow the pattern in which they are called.
| @@ -163,7 +159,6 @@ def _load_reporter_by_class(reporter_class: str) -> type: | |||
| # pylint: disable=too-many-instance-attributes,too-many-public-methods | |||
| class PyLinter( | |||
| config.OptionsManagerMixIn, | |||
| MessagesHandlerMixIn, | |||
| ignore_unknown: bool = False, | ||
| ) -> None: | ||
| """Do some tests and then iterate over message defintions to set state""" | ||
| assert scope in ("package", "module") |
There was a problem hiding this comment.
Maybe we could make scope an enum later.
There was a problem hiding this comment.
Never used those, but I think this might indeed be handy. I know some other places where this might be useful!
| def process_module(self, node: nodes.Module) -> None: | ||
| """Inspect the source file to find messages activated or deactivated by id.""" | ||
| managed_msgs = MessagesHandlerMixIn.get_by_id_managed_msgs() | ||
| managed_msgs = self._get_by_id_managed_msgs() |
There was a problem hiding this comment.
Maybe there's a better way to handle the "meta" check that needs information not accessible for a classical checker. There are some of those and we do not have a good system for them. What do you think @DanielNoord ? Maybe a MetaChecker class with access to message store and other information found only in Pylinter or MessageHandlerMixin ?
There was a problem hiding this comment.
Can you give me an example? I'm not quite sure what you are referring to.
In any case, I think we will always struggle with extracting classes and attributes from PyLinter as the design of pylint relies heavily on modifying the attributes of a (or the) PyLinter class. This is also what prompted this merge, but also what will likely warrant a merge of OptionsManagerMixIn.
I thought about creating helper functions which get PyLinter passed as argument (instead of using self) to allow some methods to be moved to a separate file. However, this does not seem like an "intended" use of Python and I can "foresee unforeseen problems in the future" (similar to how extracting into MixIn classes seemed logical some time ago).
There was a problem hiding this comment.
In fact it's all message that are added with add_message in Pylinter, MessageHandlerMixin or OptionManagerMixin instead of being their own Checker class (ie exhaustive list if you search for add_message in those classes's files). warnings related to bad option in configuration, useless-suppression, etc. This force Pylinter and co. to be a Checker itself.
There was a problem hiding this comment.
Ah yeah! Sounds like a good idea (although not for this PR). If you open an issue I might be able to look into it in the near future!
There was a problem hiding this comment.
You're a machine Daniel 😄 ! Created #5156 :) ... but take it easy !
Type of Changes
Description
This closes #5108 and is the final PR in this series. Little bit larger than I would have liked, but I'll annotate all changes.
This also allows fully ticking of
pylint/messagefrom #2079. 🚀 Runningmypy --strict pylint/messageonly returns two warnings which should disappear after other parts of the code have been typed.