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

stubgen: Allow aliases below the top level #14388

Merged
merged 2 commits into from
Jan 30, 2023

Conversation

chadrik
Copy link
Contributor

@chadrik chadrik commented Jan 4, 2023

(Explain how this PR changes mypy.)

stubgen currently allows a variable to be an alias to another, but only at the module level.

For example:

def func() -> int:
    return 2

aliased_func = func

class Foo:
    def meth() -> int:
        return 2
    
    aliased_meth = meth

produces:

def func() -> int: ...
aliased_func = func

class Foo:
    def meth() -> int: ...
    aliased_meth: Incomplete   # <--- should be  `aliased_meth = meth`

At the class level, these aliased variables are marked as Incomplete.

In my experience, enabling aliases below the top level produces better stubs.

I'm not sure what the original reservations were around use of these aliases. If the concerns about class-level aliases are still valid, I can add an option to enable them.

@chadrik chadrik force-pushed the stubgen-aliases branch 3 times, most recently from 6e2a9f7 to 42af1f1 Compare January 7, 2023 19:12
@chadrik
Copy link
Contributor Author

chadrik commented Jan 7, 2023

@hauntsaninja do you mind giving this a review?

@chadrik
Copy link
Contributor Author

chadrik commented Jan 21, 2023

Anyone have a minute for this PR?

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks, this looks reasonable.

Can we just make this the default and not have it be configurable? Could you also add some assignment statements inside methods in the test case to prove that they don't get included.

@chadrik
Copy link
Contributor Author

chadrik commented Jan 30, 2023

Can we just make this the default and not have it be configurable?

I was inclined to do that, but I don't know why it was specifically limited to module aliases in the first place. I can't see the scenario where that's preferable. I'm happy to delegate that decision to you or anyone else in the mypy crew.

Could you also add some assignment statements inside methods in the test case to prove that they don't get included.

sure.

@hauntsaninja
Copy link
Collaborator

Let's make it the default

@chadrik
Copy link
Contributor Author

chadrik commented Jan 30, 2023

I backed out the commit that added the cli arg, and added a function level alias to the test. waiting for CI tests to pass.

@chadrik
Copy link
Contributor Author

chadrik commented Jan 30, 2023

ready to go!

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thanks!

@hauntsaninja hauntsaninja merged commit 7c14eba into python:master Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants