Skip to content

Consistent quote wrapping around MessageBuilder.format as per issue #3409 #3430

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

Closed
wants to merge 28 commits into from

Conversation

mistermocha
Copy link

Tidying up inconsistent quotes around returned types for formatting. See #3409 for background.

Brian Weber and others added 20 commits May 23, 2017 15:01
This was something I was looking for in the quick start, and didn't find
clearly stated anywhere in the README.
Fixes python#3308  

This PR adds better processing of "synthetic" types (``NewType``, ``NamedTuple``, ``TypedDict``) to the third pass and moves some processing from the second to the third pass.
* Add note for diff inffered and return type

* Fix conditions for note

* Move tests to newline

* Style improvements and more detailed message

* Fix tests

* Move message function to messages

* More test cases

* Test different sequence

* Cleaner separate of test cases
* Add type checking plugin support for functions

The plugins allow implementing special-case logic for
inferring the return type of certain functions with
tricky signatures such as `open` in Python 3.

Include plugins for `open` and `contextlib.contextmanager`.

Some design considerations:

- The plugins have direct access to mypy internals. The
  idea is that most plugins will be included with mypy
  so mypy maintainers can update the plugins as needed.

- User-maintained plugins are currently not supported but
  could be added in the future. However, the intention is
  to not have a stable plugin API, at least initially.
  User-maintained plugins would have to track mypy internal
  API changes. Later on, we may decide to provide a more
  stable API if there seems to be a significant need. The
  preferred way would still be to keep plugins in the
  mypy repo.

* Add test case for additional special cases

* Fix handling of arguments other than simple positional ones

Also add comments and some defensive checks.
Fix python#3393

This results in pytest output being shown as a passthrough at the beginning of runtests.py.
Use entry_points instead of custom scripts (but custom scripts remain in case they are still useful).
Use case: type hierarchy of IDs, all of which are integers at runtime, but we
want to type-check distinctions between e.g. "id of User" and "id of Media",
while still allowing some functions to take "any ID" (which is still a more
specific type than "any integer").
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks for powering through all the tests! Just two tiny nits.

@@ -241,6 +241,12 @@ def format_simple(self, typ: Type, verbosity: int = 0) -> str:
None -> None
callable type -> "" (empty string)
"""
ret = self._format_simple(typ, verbosity)
if ret == "Module":
Copy link
Member

Choose a reason for hiding this comment

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

Should really use 'Module' here. (Also I really detest the messages "Module does not blah" -- I always need to know the module name. But that's a separate concern.)

Copy link
Author

Choose a reason for hiding this comment

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

Clarify... are you suggesting single quotes over double quotes?

Copy link
Member

Choose a reason for hiding this comment

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

Only one this one line in the source code. (In the output I am fine with the double quotes.) But the line of code that generates the special value 'Modules' also uses single quotes, as does most of the surrounding code, so I'd like this line to be consistent.

if len(s) < 400:
return s
else:
return 'union type ({} items)'.format(len(items))
elif isinstance(typ, NoneTyp):
return 'None'
elif isinstance(typ, AnyType):
return '"Any"'
return 'Any'
elif isinstance(typ, DeletedType):
return '<deleted>'
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I think it would look ugly to have "<deleted>" in the error messages, and ditto for "<nothing>" below. Maybe the code that adds the quotes should make an exception for strings starting with < and ending with >.

Copy link
Author

Choose a reason for hiding this comment

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

<deleted> was not part of this diff. Can we bump to another? This one is already a behemoth of test changes.

Copy link
Member

Choose a reason for hiding this comment

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

But IIUC this diff would change <deleted> into "<deleted>" which I think is wrong. So it's part of the diff. And there seem to be no tests with <deleted> so there's no extra burden there. (Similar for <nothing>.)

@gvanrossum gvanrossum self-assigned this May 29, 2017
@gvanrossum
Copy link
Member

PS. Who's Brian Weber (bweber@twitter.com)? The commits are by him but the PR is by mistermocha. Are you the same person?

@gvanrossum
Copy link
Member

Also please resolve the merge conflict.

gvanrossum and others added 6 commits May 28, 2017 20:17
This gets rid of code duplication and makes sure mypy works with the released version of `typing`. This doesn't fix any immediate issue, but I saw some comments in the bug tracker saying that we should be able to get rid of `lib-typing` eventually.

There is a minor downside: To run mypy tests locally, you will now have to have a Python 2.7 installed that has `typing` available.
@mistermocha
Copy link
Author

Yes, I'm bweber@twitter.com as well as this account for @mistermocha. I have been working off of my work laptop for sprints. so my ~/.gitconfig has my work email.

Test conflicts galore. After a pull/rebase/merge-conflict-resolve, I updated test data and got this:

Expected:
  ok                                            (diff)
Actual:
  Traceback (most recent call last):            (diff)
    File "_testPrintFunctionWithFileArg_python2.py", line 2, in <module> (diff)
      import typing                             (diff)
  ImportError: No module named typing           (diff)

... mutiple times. This doesn't seem right.

@emmatyping
Copy link
Member

After recent commits you need to install typing for Python 2 on your machine.

@gvanrossum
Copy link
Member

Yes, I'm bweber@twitter.com as well as this account for @mistermocha.

Since you've now been outed you might as well update the full name and email on the GitHub account (AFAIK GitHub lets you register multiple email addresses).

@gvanrossum
Copy link
Member

I don't know what you did, but you now have a bunch of unrelated commits in this PR. I expect you can fix that using rebase (but I don't know your setup so I can't give you more specific advice).

@gvanrossum
Copy link
Member

Blocked until that can be fixed.

Possibly you could save your diffs in an old-fashioned patch file outside your repo, get rid of your clone, get rid of the fork, create a new fork, clone that, and re-apply your patches using git patch. Depending on your network bandwidth that might be quicker than trying to recover...

@ilevkivskyi
Copy link
Member

@mistermocha are you still working on this? this would help other PRs.

@mistermocha
Copy link
Author

mistermocha commented Aug 22, 2017 via email

@OddBloke
Copy link
Contributor

I've been taking a look at this, and I've resolved most of the conflicts. Will push up a branch later today.

@OddBloke
Copy link
Contributor

That branch is pushed and I've proposed it for merge at #3873.

@ilevkivskyi
Copy link
Member

OK, I am closing this in favor of #3873

@ilevkivskyi
Copy link
Member

@mistermocha Thanks for your efforts! This PR was very helpful as a starting point.

@mistermocha
Copy link
Author

mistermocha commented Aug 25, 2017 via email

ilevkivskyi pushed a commit that referenced this pull request Aug 30, 2017
This is a refreshed version of #3430 (credits @mistermocha). Fixes #3409.
Tidying up inconsistent quotes around returned types for formatting.
See #3409 for background.
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.