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 --list-msgs & --list-msgs-enabled #4793

Merged
merged 3 commits into from Aug 3, 2021

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Type of Changes

Type
✨ New feature
🔨 Refactoring

Description

Both options now show which messages can't be emitted with the current interpreter. This makes function more like their name implies. Code clearly shows in which order/format the messages are emitted.
This closes #4778

Both options now show which messages can't be emitted with the
current interpreter. This makes function more like their name implies.
This closes pylint-dev#4778
@coveralls
Copy link

coveralls commented Aug 3, 2021

Coverage Status

Coverage increased (+0.01%) to 92.587% when pulling 09b6f99 on DanielNoord:list-msgs into 741276e on PyCQA:main.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Nice job ! Could we refactor a little further and create a common function to recover emitable/non-emittable ? This way list_messages_enabled would loop on the result of this function and the other one would use them directly.

enabled = []
disabled = []
non_emittable = []
for message in self.msgs_store.messages:
Copy link
Member

Choose a reason for hiding this comment

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

Nice refactor 👍

@DanielNoord
Copy link
Collaborator Author

Where should I put this common function? Is there a good place for utils function for files in different directories?

Thinking of:

def find_emittable_messages(
    messages: List[MessageDefinition],
) -> Tuple[List[MessageDefinition], List[ValuesView[MessageDefinition]]]:
    emittable = []
    non_emittable = []
    for message in messages:
        if message.may_be_emitted():
            emittable.append(message)
        else:
            non_emittable.append(message)
    return emittable, non_emittable

@Pierre-Sassoulas
Copy link
Member

MessageDefinitionStore is the logical place so you don't have to provide the messages (they're already a class attribute).

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you !

@Pierre-Sassoulas Pierre-Sassoulas merged commit a71cfe1 into pylint-dev:main Aug 3, 2021
@DanielNoord DanielNoord deleted the list-msgs branch August 3, 2021 21:21
@scop
Copy link
Contributor

scop commented Aug 5, 2021

FYI, my recent pylint bash completion PR at scop/bash-completion#562 is what triggered to me to file #4778. Looking at the code here it seems my scraper would be still doing the right thing after these changes, cool.

The scraper is far from pretty, but all things considered I don't think we have a better choice at the moment, and if we want to support old pylint versions out there, new additions of e.g. better machine parseable outputs for these in new releases wouldn't help either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--list-msgs and --list-msgs-enabled output different sets of messages
4 participants