Skip to content

Fix location metadata on backslash/unicode escape lints #4003

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
Merged
merged 2 commits into from
Jan 4, 2021
Merged

Fix location metadata on backslash/unicode escape lints #4003

merged 2 commits into from
Jan 4, 2021

Conversation

msuozzo
Copy link
Contributor

@msuozzo msuozzo commented Dec 31, 2020 •
edited
Loading

The existing logic does not account for the start-of-line to start-of-string offset in the column and does not adjust the line to reflect where the actual backslash occurs.

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

Type of Changes

Type
✓ 🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Related Issue

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.008%) to 90.652% when pulling 99f0758 on msuozzo:patch-1 into f95bbac on PyCQA:master.

@coveralls
Copy link

coveralls commented Dec 31, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 90.7% when pulling a2d25ba on msuozzo:patch-1 into f3fd5ab on PyCQA:master.

Pierre-Sassoulas
Pierre-Sassoulas requested changes Jan 1, 2021
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.

Thank you for the merge request ! Could you also add an example of code that was failing before this change in tests/functional so we do not regress later ?

@msuozzo msuozzo changed the title Fix column metadata on backslash lint checks Fix location metadata on backslash/unicode escape lints Jan 3, 2021
@msuozzo
Copy link
Contributor Author

msuozzo commented Jan 3, 2021

Alright added some tests and added a fix to the line metadata to this PR.

There is currently no functional test facility for column but this should be feasible to add. Considering that this would mean modifying all existing functional tests, I think it's best left for a follow-on change.

@msuozzo
Copy link
Contributor Author

msuozzo commented Jan 3, 2021

Cleaned up commit history, as well.

This was referenced Jan 3, 2021
Merged
Merged
@Pierre-Sassoulas
Copy link
Member

Wow, you're right the functional test do not check for column ! Well, I made the required change so you can implement this properly : we'll move the legacy backlash functional test to the new functional test framework in : #4012. Also, we're going to add this column check to functional tests in #4013. Feel free to review them as this will impact what you do here :)

@msuozzo
Copy link
Contributor Author

msuozzo commented Jan 3, 2021

Alright I've rebased the fix for the legacy test to the new functional one.

Unfortunately, your PR seems to run into the issue that py3.8 AST has different (read: more accurate) column metadata than do previous versions. Maybe add a config param to all tests to not check columns before py3.8?

msuozzo added 2 commits January 4, 2021 11:50
@msuozzo
Fix location metadata on backslash lint checks
56f5ee5 
The existing column offset logic does not account for the start-of-line
to start-of-string offset nor does the line logic reflect the actual
line number of the error for multi-line strings.
@msuozzo
Add test for anomalous-backslash-in-string.
a2d25ba
@Pierre-Sassoulas
Copy link
Member

Ok, testing columns is not as easy as I thought, it might take some time to make it work from python interpreters above 3.8. I'll probably wait for #3959 and #3965 to be resolved.

Pierre-Sassoulas
Pierre-Sassoulas approved these changes Jan 4, 2021
View reviewed changes
tests/functional/e/excess_escapes.py
@@ -25,7 +25,8 @@
ESCAPE_UNICODE = "\\\\n"

# Bad docstring
"""Even in a docstring # [anomalous-backslash-in-string]
# +3:[anomalous-backslash-in-string]
Copy link
Member

Choose a reason for hiding this comment

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

👍

@Pierre-Sassoulas Pierre-Sassoulas merged commit e2c06c0 into pylint-dev:master Jan 4, 2021
@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Jan 4, 2021
@Pierre-Sassoulas
Copy link
Member

Pretty useful MR, I remember having to search for the offending backslash in the whole docstring. This will make a lot of lives easier :) Congratulation on your first contribution to pylint !

@msuozzo msuozzo deleted the patch-1 branch January 4, 2021 23:54
@msuozzo
Copy link
Contributor Author

msuozzo commented Jan 4, 2021

Thanks so much @Pierre-Sassoulas! And thanks for the review and for implementing column-based testing (even though it can't be merged immediately)!

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
Enhancement ✨ Improvement to a component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@msuozzo @coveralls @Pierre-Sassoulas