Skip to content

Conversation

mcepl
Copy link
Contributor

@mcepl mcepl commented Oct 24, 2017

Fixes #295

@mcepl mcepl changed the title WIP: stub for imaplib (2and3) stub for imaplib (2and3) Oct 25, 2017
@mcepl
Copy link
Contributor Author

mcepl commented Oct 25, 2017

OK, I give up. Could I ask for help, please, how to fix these remaining validation errors? What does Invalid type "typing.AnyStr" mean? I am lost.

@gvanrossum
Copy link
Member

You can't use AnyStr the way you tried. If you replace it with Union[bytes, Text] it should have the effect you want. (AnyStr is really a type variable, so only applicable in generic class and function definitions.)

@mcepl
Copy link
Contributor Author

mcepl commented Oct 25, 2017

Thanks for the hint. So, what do you think?

@gvanrossum
Copy link
Member

gvanrossum commented Oct 25, 2017 via email

@mcepl
Copy link
Contributor Author

mcepl commented Oct 25, 2017

Of course, I did. Sorry, I live in other worlds where it is the right thing to do. Did I miss it in the Contributing.md? Actually, even you are pointing to this blogpost. You should probably clarify it somewhere, if you don't want clueless newbies to screw up your workflow. Sorry again.

@gvanrossum
Copy link
Member

The recommendation to squash commits is for core devs who are merging PRs -- we should use the Squash-and-merge button. (As I will do shortly.) But PR authors should not squash.

@mcepl
Copy link
Contributor Author

mcepl commented Oct 26, 2017

we should use the Squash-and-merge button. (As I will do shortly.)

OK, then I really don't understand. So, in the end you want to have just one commit per topic? Why then insist on messy squashed commit messages instead of the hand-crafted ones? I don't see any other difference.

@JelleZijlstra
Copy link
Member

I believe squashing commits tends to mess up the attribution of previous inline review comments.

@mcepl mcepl deleted the imaplib branch October 26, 2017 16:36
@mcepl
Copy link
Contributor Author

mcepl commented Oct 26, 2017

OK, that's true. Not completely persuaded, but that makes at least some sense.

@gvanrossum
Copy link
Member

gvanrossum commented Oct 26, 2017 via email

@mcepl
Copy link
Contributor Author

mcepl commented Oct 27, 2017

I am sorry, I was not trying to argue. Anyway, shutting up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants