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 future=True to frame calls #1305

Merged
merged 36 commits into from
Dec 29, 2021
Merged

Add future=True to frame calls #1305

merged 36 commits into from
Dec 29, 2021

Conversation

tushar-deepsource
Copy link
Contributor

@tushar-deepsource tushar-deepsource commented Dec 20, 2021 •
edited
Loading

This builds upon #1263, the diff should be smaller once that is merged.

Type of Changes

Type
✓ 🔨 Refactoring

tushar-deepsource and others added 30 commits December 20, 2021 14:06
@tushar-deepsource
Fix frame() error on inferred node
fff4db4
@tushar-deepsource
Update node_ng.py
2cab065
@pre-commit-ci @tushar-deepsource
[pre-commit.ci] auto fixes from pre-commit.com hooks
8952c64 
for more information, see https://pre-commit.ci
@tushar-deepsource
Update node_ng.py
4ea13b2
@tushar-deepsource
Add FrameMissing exception
42ce9f4
@pre-commit-ci @tushar-deepsource
[pre-commit.ci] auto fixes from pre-commit.com hooks
4629c93 
for more information, see https://pre-commit.ci
@tushar-deepsource
Fix flake8 lint
9b2c483
@tushar-deepsource
Fix pylint error
27c1707
@tushar-deepsource
Remove unnecessary isinstance
1b59f00
@tushar-deepsource
Add types and remove exception type
54bcb59
@tushar-deepsource
Add visit_unknown
7bee391
@tushar-deepsource
Formatting
9e55d84
@Pierre-Sassoulas @tushar-deepsource
Upgrade pylint to 2.12.2 (pylint-dev#1297)
9da479c 
* Upgrade pylint to 2.12.2

* Default Python in the CI is now python 3.8

* Remove useless suppression for python 3.8

* Disable no-member for false positive with zipimport
@tushar-deepsource
Fix flake8 lint
9fb55c5
@tushar-deepsource
Add types and remove exception type
dac07c6
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
3163665 
for more information, see https://pre-commit.ci
@tushar-deepsource
Remove unrelated code
08014ed
@tushar-deepsource
Hack to trigger ci
67281cb
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
3bee4f5 
for more information, see https://pre-commit.ci
@tushar-deepsource
Remove unused import
db901ce
@tushar-deepsource
something
a01e510
@tushar-deepsource
Move import to TYPE_CHECKING block
410445b
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
1d6dc24 
for more information, see https://pre-commit.ci
@tushar-deepsource
Unused import
a3b2db9
@tushar-deepsource
ci
4a1a4ad
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
9efdc91 
for more information, see https://pre-commit.ci
@tushar-deepsource
Merge branch 'main' of https://github.com/pycqa/astroid into tushar-d…
492a20d 
…eepsource-patch-1
@tushar-deepsource
Revert noreturn change
fef9154
@tushar-deepsource
Add future=True to all frsme() calls
b0fab08
@tushar-deepsource
Merge branch 'main' of https://github.com/pycqa/astroid into frame-fu…
5754615 
…ture
jacobtylerwalls
jacobtylerwalls reviewed Dec 24, 2021
View reviewed changes
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

Hi, just some comments on the deprecation warning itself.

astroid/nodes/node_ng.py Outdated Show resolved Hide resolved
astroid/nodes/node_ng.py Outdated Show resolved Hide resolved
@tushar-deepsource
Copy link
Contributor Author

This seems like an unrelated failure. Flaky test?

@Pierre-Sassoulas
Copy link
Member

This seems like an unrelated failure. Flaky test?

Yes, we had the issue for other pipelines. I don't know what the problem is yet. Probably due to the new release of python 3.10.

@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Dec 24, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.10.0 milestone Dec 24, 2021
@tushar-deepsource
Copy link
Contributor Author

Resolved merge conflict, should be good

Pierre-Sassoulas
Pierre-Sassoulas approved these changes Dec 29, 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.

LGTM. Can we merge this in 2.9.1 @DanielNoord ?

@DanielNoord
Copy link
Collaborator

DanielNoord commented Dec 29, 2021 •
edited
Loading

I haven't look at the other changes to actual code yet, but note that in #1217 and #1235 we did not change the existing tests but added additional tests with the future keyword. This guarantees that previous behaviour has not been changed.
See:
https://github.com/PyCQA/astroid/blob/7afd696520f70a46334adea6d39f7150379198f3/tests/unittest_scoped_nodes.py#L824-L825

It is a bit nitpicky, but might be good to do here as well. It also means we can remove the lines with future in 3.0 and the diff wouldn't change for the original tests.

Edit: The changes itself can go in 2.9.1 I think. Based on the tests no actual behaviour changed so it should be fine. We did the same for statement (ie., assume that no critical functionality was changed by adding future).

@Pierre-Sassoulas
Copy link
Member

It is a bit nitpicky, but might be good to do here as well.

Yeah I hesitated doing that comment too. Having test for the old behavior is nice but I missed the fact that we can remove the lines with future in 3.0 and the diff wouldn't change for the original tests. and it's a very nice thing to be able to do. 👍

@tusharsadhwani
Copy link
Contributor

oh that's true. I'll fix that

@tushar-deepsource
Copy link
Contributor Author

All done

cdce8p
cdce8p approved these changes Dec 29, 2021
View reviewed changes
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.

LGTM! Test failures are again unrelated

@cdce8p cdce8p modified the milestones: 2.10.0, 2.9.1 Dec 29, 2021
Pierre-Sassoulas
Pierre-Sassoulas approved these changes Dec 29, 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.

I'll let Daniel merge after his review :)

@cdce8p cdce8p requested a review from DanielNoord December 29, 2021 12:14
@DanielNoord DanielNoord merged commit aa4f5be into pylint-dev:main Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobtylerwalls jacobtylerwalls jacobtylerwalls left review comments

@cdce8p cdce8p cdce8p approved these changes

@DanielNoord DanielNoord DanielNoord approved these changes

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

Assignees
No one assigned
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Milestone
2.9.1
Development

Successfully merging this pull request may close these issues.
6 participants
@tushar-deepsource @Pierre-Sassoulas @DanielNoord @tusharsadhwani @cdce8p @jacobtylerwalls