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

Update regression tests due to improved inference in astroid #4325

Closed

Conversation

nelfin
Copy link
Contributor

@nelfin nelfin commented Apr 8, 2021

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

Depends pylint-dev/astroid#927. This custom logger subclass is now correctly
inferred and so will raise the appropriate warnings.

Type of Changes

Type
🐛 Bug fix

Related Issue

pylint-dev/astroid#926

@nelfin
Copy link
Contributor Author

nelfin commented Apr 8, 2021

I'm assuming that this probably doesn't need a changelog entry

@nelfin nelfin force-pushed the fix/astroid-926-regression-tests branch from d188c98 to a62cb39 Compare April 8, 2021 23:36
@nelfin
Copy link
Contributor Author

nelfin commented Apr 8, 2021

(Note that these tests are XFAIL until the astroid changes land)

@cdce8p cdce8p added the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Apr 9, 2021
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

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

I just ran your changes against your astroid PR and noticed two small issue with the column numbers.
Aside from that, I agree these lines should be flagged.

tests/functional/l/logging_format_interpolation.txt Outdated Show resolved Hide resolved
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.

👍

@cdce8p cdce8p removed the Needs astroid update Needs an astroid update (probably a release too) before being mergable label May 11, 2021
@cdce8p cdce8p added the Needs astroid update Needs an astroid update (probably a release too) before being mergable label May 12, 2021
@cdce8p
Copy link
Member

cdce8p commented May 12, 2021

@nelfin Just to be sure, it's only in draft mode because the astroid PR wasn't merged previously or do you plan to add anything else here?
Edit: Seems like there is also a small merge conflict

nelfin and others added 2 commits May 13, 2021 07:36
Depends astroid/pylint-dev#927. This custom logger subclass is now correctly
inferred and so will raise the appropriate warnings.
Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>
@nelfin nelfin force-pushed the fix/astroid-926-regression-tests branch from 264d325 to a660f27 Compare May 12, 2021 21:37
@nelfin
Copy link
Contributor Author

nelfin commented May 12, 2021

@nelfin Just to be sure, it's only in draft mode because the astroid PR wasn't merged previously or do you plan to add anything else here?

Yep, that's exactly the case. I left it in draft so it wasn't merged beforehand (but I suppose the failing tests should have been enough).

Edit: Seems like there is also a small merge conflict

Fixed

@nelfin nelfin marked this pull request as ready for review May 12, 2021 21:37
@cdce8p cdce8p added this to the 2.8.3 milestone May 24, 2021
@cdce8p
Copy link
Member

cdce8p commented May 30, 2021

Already merged with #4523: 0e51660
Thanks again @nelfin!

@cdce8p cdce8p closed this May 30, 2021
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.8.3, 2.9.0 May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 Needs astroid update Needs an astroid update (probably a release too) before being mergable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants