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

Fix typing alias #916

Merged
merged 16 commits into from
Feb 28, 2021
Merged

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Feb 28, 2021

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

Replaces #913
CC: @hippo91 @Pierre-Sassoulas

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes #905
Closes pylint-dev/pylint#4093
Closes pylint-dev/pylint#4131
Closes pylint-dev/pylint#4145
Closes pylint-dev/pylint#3247
Closes pylint-dev/pylint#2717

@cdce8p
Copy link
Member Author

cdce8p commented Feb 28, 2021

@Pierre-Sassoulas It would be great if you could look at over one last time. Just to make sure I didn't miss something obvious. I'm running some tests at the moment to check if everything is good.

@Pierre-Sassoulas Pierre-Sassoulas mentioned this pull request Feb 28, 2021
2 tasks
@Pierre-Sassoulas
Copy link
Member

This could benefit from a rewrite using pre-commit for each commit: git rebase upstream/master -i --exec="pre-commit run --all-fles". Also it's the last thing we need to merge for 2.5.1, right ?

@Pierre-Sassoulas
Copy link
Member

Or maybe we just squash.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 28, 2021

Or maybe we just squash.

I would do a squash merge

Also it's the last thing we need to merge for 2.5.1, right ?

The version needs to be updated, but otherwise I believe that's it.

--

I noticed a moment ago the the Python 3.7 test is failing. I'll have to check that first.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 28, 2021

Strange, Appveyor was fine and Travis had an error for 3.7.
Anyway I pushed a small change which should trigger the CI tests, although it doesn't change much.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 28, 2021

Unfortunately some more errors, this time with pylint: https://github.com/cdce8p/pylint/runs/1999076587?check_suite_focus=true#step:6:205
This seems to be more serious, so I would like to address it before merging.

and "ABCMeta" == meta.basenames[0]
and meta.locals.get("__getitem__") is not None
)
assert isinstance(meta, nodes.ClassDef)
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I really hope all the tests pass now. Was a bit more difficult to fix than I expected.

Copy link
Member Author

@cdce8p cdce8p Feb 28, 2021

Choose a reason for hiding this comment

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

@Pierre-Sassoulas Can you reproduce the Travis issue. I unfortunately can't 😞
https://travis-ci.org/github/PyCQA/astroid/jobs/760830892

Copy link
Member

Choose a reason for hiding this comment

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

Hmm it's strange, on 2391061 I have an error but not the same than for Travis

____ ExceptionModelTest.test_oserror __

self = <tests.unittest_object_model.ExceptionModelTest testMethod=test_oserror>

    def test_oserror(self):
        ast_nodes = builder.extract_node(
            """
        try:
            raise OSError("a")
        except OSError as err:
           err.filename #@
           err.filename2 #@
           err.errno #@
        """
        )
        expected_values = ["", "", 0]
        for node, value in zip(ast_nodes, expected_values):
            inferred = next(node.infer())
>           assert isinstance(inferred, astroid.Const)
E           AssertionError: assert False
E            +  where False = isinstance(Uninferable, <class 'astroid.node_classes.Const'>)
E            +    where <class 'astroid.node_classes.Const'> = astroid.Const

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. Should I just change the required version to 3.8?
Since it works with AppVeyor Link, I would assume that this is a Travis issue.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 28, 2021

@Pierre-Sassoulas I think it's done. Nothing unexpected with any tests.
I'll prepare pylint-dev/pylint#4152 for the astroid update in pylint.

@Pierre-Sassoulas
Copy link
Member

Sounds good, I'll release astroid 2.5.1 asap :)

@Pierre-Sassoulas Pierre-Sassoulas merged commit 181642f into pylint-dev:master Feb 28, 2021
@cdce8p cdce8p deleted the fix-typing-alias branch February 28, 2021 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment