Skip to content

CLN,TYP Remove string return annotations #39174

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

Merged

Conversation

MarcoGorelli
Copy link
Member

@MarcoGorelli MarcoGorelli commented Jan 14, 2021

  • Ensure all linting tests pass, see here for how to run them

this was just done with

    parser = argparse.ArgumentParser()
    parser.add_argument('paths', nargs='*', type=Path)
    args = parser.parse_args()
    for path in args.paths:
        text = path.read_text()
        text = re.sub(r'-> "(\w+)"', '-> \\1', text)
        path.write_text(text)

There's other to do (which I'll get to in other PRs), but this already gets rid of a big load of them

@MarcoGorelli MarcoGorelli force-pushed the future-annotations-returns branch from e14f02d to 47dfee1 Compare January 14, 2021 16:51
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

nice. merge on green.

@simonjayhawkins simonjayhawkins added Clean Typing type annotations, mypy/pyright type checking labels Jan 14, 2021
@simonjayhawkins simonjayhawkins added this to the 1.3 milestone Jan 14, 2021
@jbrockmendel
Copy link
Member

Not a huge deal, but I like the string versions since they make the dependency structure clearer, i.e. I’m not going to incorrectly infer that Series is in the namespace.

@MarcoGorelli
Copy link
Member Author

Sure, but you can't click and go to a definition if it's a string version (this is my main reason for wanting to make this kind of change)

I think it only makes the dependency structure clearer if string ones are used consistently (currently there's a mix) - up to you anyway, my preference here isn't too strong

@simonjayhawkins
Copy link
Member

Sure, but you can't click and go to a definition if it's a string version (this is my main reason for wanting to make this kind of change)

some discussion in #36034 (comment)

I think it only makes the dependency structure clearer if string ones are used consistently (currently there's a mix)

agreed. You can (and people have) quote types even if the type is imported at runtine, or quote the whole type expression such as "List[grouper.Grouping]" which the PR resolves.

"we" agreed (#36034 (comment)) on going this route. Do we want another discussion?

@jbrockmendel
Copy link
Member

Do we want another discussion?

Nope, I'm pretty happy with my "defer to Simon when it comes to annotations" policy.

@MarcoGorelli
Copy link
Member Author

OK, merging following #39174 (review) and #39174 (comment)

@MarcoGorelli MarcoGorelli merged commit f51547c into pandas-dev:master Jan 16, 2021
@MarcoGorelli MarcoGorelli deleted the future-annotations-returns branch January 16, 2021 11:15
@simonjayhawkins
Copy link
Member

Thanks @MarcoGorelli

luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021
* remove string return annotations

* fixup conflict

* fixup merge conflict
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants