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.

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

add test and expl for line-too-long useless-supp FP #7887

Merged
merged 3 commits into from Dec 3, 2022
Merged

add test and expl for line-too-long useless-supp FP #7887

merged 3 commits into from Dec 3, 2022

Conversation

clavedeluna
Copy link
Collaborator

Type of Changes

Type
✓ 📜 Docs

Description

Closes #3368

Pierre-Sassoulas
Pierre-Sassoulas requested changes Dec 2, 2022
View reviewed changes
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.

Some wording, and I think it's better to xfail the expected behavior rather than crystalize the wrong behavior

doc/data/messages/l/line-too-long/details.rst Outdated Show resolved Hide resolved
doc/whatsnew/fragments/3368.false_positive Outdated Show resolved Hide resolved
tests/test_self.py Outdated Show resolved Hide resolved
tests/data/line_too_long_no_code.py Outdated Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.16.0 milestone Dec 2, 2022
@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code backport maintenance/2.15.x labels Dec 2, 2022
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.16.0, 2.15.7 Dec 2, 2022
@clavedeluna @Pierre-Sassoulas
Apply suggestions from code review
8ab6f4b 
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
@DanielNoord
Copy link
Collaborator

Some wording, and I think it's better to xfail the expected behavior rather than crystalize the wrong behavior

Isn't the issue here that we can't know for sure that the xfail is because of the original reason? If we break something else the test will still fail. I like documenting the poor behaviour, even though it is not perfect. At least that way we know what is/was expected behaviour rather than "something in this test will fail".

@Pierre-Sassoulas
Copy link
Member

Sure, in theory something else could fail, but this is a one line file with only the example, I think we're safe. There's a lot of time where we asked ourselves why something "obviously" wrong was in the functional tests.

@DanielNoord
Copy link
Collaborator

Sure, in theory something else could fail, but this is a one line file with only the example, I think we're safe. There's a lot of time where we asked ourselves why something "obviously" wrong was in the functional tests.

Can't we add:

# This test serves mostly to document current behaviour.
# If you manage to make it fail and remove the useless-suppression
# warning please contact Daniel to get your
# celebratory reward (and well done 👍🏻)

@Pierre-Sassoulas
Copy link
Member

Sounds great, what's the reward going to be though ? 😄

@DanielNoord
Copy link
Collaborator

10 internet points, a beer if they ever meet me in person and an extra speedy review of a PR of choice.
Who wouldn't want to cash in that reward?

@clavedeluna
Copy link
Collaborator Author

clavedeluna commented Dec 2, 2022 •
edited

Sounds great, what's the reward going to be though ? 😄

Are you agreeing to keeping the original test, so I can revert the latest commit minus doc changes, correct?

@Pierre-Sassoulas
Copy link
Member

Sure !

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3605491429

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.482%
Totals Coverage Status
Change from base Build 3587408370: 0.0%
Covered Lines: 17645
Relevant Lines: 18480
💛 - Coveralls

@Pierre-Sassoulas Pierre-Sassoulas merged commit 30c931f into pylint-dev:main Dec 3, 2022
@github-actions github-actions bot mentioned this pull request Dec 3, 2022
Merged
github-actions bot pushed a commit that referenced this pull request Dec 3, 2022
@clavedeluna @github-actions
add test and expl for line-too-long useless-supp FP (#7887)
98bfc14 
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
(cherry picked from commit 30c931f)
Pierre-Sassoulas pushed a commit that referenced this pull request Dec 4, 2022
@clavedeluna @Pierre-Sassoulas
add test and expl for line-too-long useless-supp FP (#7887)
ad51069 
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
(cherry picked from commit 30c931f)
Pierre-Sassoulas pushed a commit that referenced this pull request Dec 4, 2022
@clavedeluna @Pierre-Sassoulas
add test and expl for line-too-long useless-supp FP (#7887)
438025d 
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
(cherry picked from commit 30c931f)
@Pierre-Sassoulas Pierre-Sassoulas mentioned this pull request Aug 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

Assignees
No one assigned
Labels
Backported Documentation 📗 False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Milestone
2.15.7
Development

Successfully merging this pull request may close these issues.

Useful 'line-too-long' suppression considered useless
4 participants
@clavedeluna @DanielNoord @Pierre-Sassoulas @coveralls