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

Bug 2957 #3076

Merged
merged 63 commits into from
Nov 19, 2019
Merged

Bug 2957 #3076

merged 63 commits into from
Nov 19, 2019

Conversation

hippo91
Copy link
Contributor

@hippo91 hippo91 commented Aug 24, 2019

Steps

  • [ * ] 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.

Description

This PR is an answer to #2957. It allows the line-too-long message to be disabled for multilines string such as docstring for example.
With this PR if a pylint:disable=line-too-long pragma is present at the end of a docstring then it is valid for the entire docstring.

Eventually if this modification is not judged usefull, i will make another PR that will just reformat the check_lines method inside pylint/checkers/format module because i had some difficulties to read and understand it.

Type of Changes

Type
🐛 Bug fix
🔨 Refactoring

Related Issue

…red when deactivating line-too-long message.
…tivated and the test of equal sign presence is move inside it
…ethod anymore but directly inside the check_lines method
@coveralls
Copy link

Coverage Status

Coverage decreased (-5.0e-05%) to 90.028% when pulling d90e6d6 on hippo91:bug_2957 into 6b3afd4 on PyCQA:master.

@coveralls
Copy link

coveralls commented Aug 24, 2019

Coverage Status

Coverage increased (+0.04%) to 89.808% when pulling 7f8e8d8 on hippo91:bug_2957 into 0ed3782 on PyCQA:master.

@hippo91
Copy link
Contributor Author

hippo91 commented Oct 20, 2019

@PCManticore no need to review this yet. I have still some corrections to make.

@hippo91
Copy link
Contributor Author

hippo91 commented Nov 11, 2019

@PCManticore i think i have finished this work. I have moved the doc toward 2.5 section and thus deleting the corresponding doc in the 2.4 section.
Thanks in advance for your review.

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.

Overall looks great, thank you for the hard work @hippo91 ! Left a bunch of minor comments, let me know what you think.

super(PragmaParserError, self).__init__(self.message)


class UnknownKeyword(PragmaParserError):
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's append Error to these exceptions to be more obvious that they are exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

if not assignment_required:
if action:
# A keyword has been found previously but doesn't support assignement
raise UnsupportedAssignment(
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need all these exceptions or if we should raise the same exception but with a corresponding message for each situation. My reasoning is that nobody will need to handle all these exceptions to verify that a pragma is broken and will result to handle just the uppermost base class. What do you think?



ATOMIC_KEYWORDS = ("disable-all", "skip-file")
MESSAGE_KEYWORDS = ("disable-msg", "enable-msg", "disable", "enable")
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a frozenset:

Suggested change
MESSAGE_KEYWORDS = ("disable-msg", "enable-msg", "disable", "enable")
MESSAGE_KEYWORDS = frozenset(("disable-msg", "enable-msg", "disable", "enable"))

pylint/lint.py Outdated
self.add_message(
"bad-option-value", args=msgid, line=start[0]
)
except (UnknownKeyword, UnsupportedAssignment) as err:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see now where we need all these exceptions. It looks to me though that two should be enough, one to convey the error for a valid, but unrecognized option, and another for a completely invalid pragma.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you @PCManticore . The changes have been done.

@@ -0,0 +1,90 @@
import pytest
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we run black over the tests directory but it seems like this file could be formatted, especially the imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! I did it thanks to black and isort.

@hippo91
Copy link
Contributor Author

hippo91 commented Nov 18, 2019

@PCManticore i did the change you requested. The CI seems to be blocked on continuous-integration/appveyor/branch and i can't get any infos to understand why. Have you got an idea?

@PCManticore PCManticore merged commit 9bdae8b into pylint-dev:master Nov 19, 2019
@PCManticore
Copy link
Contributor

@hippo91 I have no idea as well, it seems to be a misconfiguration of AppVeyor somehow, but I wasn't able to disable that particular check from AppVeyor. I'll give it another go as it is a bit frustrating that it appears for every PR.

@hippo91 hippo91 deleted the bug_2957 branch June 20, 2020 11:03
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.

4 participants