Consistent quote wrapping around MessageBuilder.format by OddBloke · Pull Request #3873 · python/mypy

OddBloke · 2017-08-25T10:58:05Z

This is a refreshed version of #3430, and fixes #3409.

As all types are now quoted, this fixes places that they would be double-quoted in the protocol error messages.

ilevkivskyi · 2017-08-25T12:55:14Z

test-data/unit/check-varargs.test

 [out]
-main:3: error: Argument 1 to "f" has incompatible type *List[A]; expected "B"
-main:4: error: Argument 2 to "f" has incompatible type *List[A]; expected "B"
+main:3: error: Argument 1 to "f" has incompatible type *"List[A]"; expected "B"


I think the star should be inside quotes for types like this.

OddBloke · 2017-08-25T13:29:43Z

Yep, agreed. Away from my laptop for the rest of the day, so I'll look at it tomorrow.

…

On Fri, 25 Aug 2017, 13:55 Ivan Levkivskyi ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test-data/unit/check-varargs.test <#3873 (comment)>: > @@ -277,13 +277,13 @@ class A: pass class B: pass [builtins fixtures/list.pyi] [out] -main:3: error: Argument 1 to "f" has incompatible type *List[A]; expected "B" -main:4: error: Argument 2 to "f" has incompatible type *List[A]; expected "B" +main:3: error: Argument 1 to "f" has incompatible type *"List[A]"; expected "B" I think the star should be inside quotes for types like this. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3873 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD1EGdO4LeDk4_FaIgrF--UUnLlC8Rlks5sbsQ1gaJpZM4PChFw> .

OddBloke · 2017-08-26T12:11:34Z

So looking at how the formatting of the star-args is done, I think the formatting methods should probably be refactored a bit.

Currently, the "public" API (.format and .format_simple) will only ever return quoted strings, which mean that callers have to remove the quotes on those strings if they want to use them for something else (like, for example, prepending * or ** inside the quotes). I think we should have additional methods that will return the "bare" formatted string (which the existing methods can then just wrap with the quoting logic).

Also, as far as I can tell, the only way that .format_simple gets called in the mypy code base is by .format (or one of the things that only .format calls), so I'll also look at dropping .format_simple in favour of just ._format_simple.

…mple This makes the quoting code a little more consistent.

Instead update MessageBuilder.format to call _format_simple directly.

This is ._format_simple renamed to reflect its new intended contract.

This replaces strip_quotes(self.format(...)) calls, which have identical effect.

This adds a bare parameter to format_distinctly, so that MessageBuilder.incompatible_argument can take care of the quoting for its special case.

OddBloke · 2017-08-27T09:28:03Z

I've done that refactoring and addressed the comment from @ilevkivskyi.

ilevkivskyi

Sorry for a delay, this looks almost ready, just few small comments.

ilevkivskyi · 2017-08-29T23:44:07Z

mypy/messages.py

-        else:
-            # Default case; we simply have to return something meaningful here.
-            return 'object'
+        Convert a type to a relatively short string suitable for error messages.


Please add an empty line after the first docstring line.

Line 189 is blank, it's just an existing blank line that the diff has kept from somewhere.

ilevkivskyi · 2017-08-29T23:45:57Z

mypy/messages.py

+        .format_bare.
+        """
+        ret = self.format_bare(typ, verbosity)
+        if ret in ['Module', 'overloaded function']:


I am not sure, but maybe add <nothing>, <deleted>, and union type ({} items)?

I've added the first two, but don't have time to add the union and tuple ones right now.

ilevkivskyi · 2017-08-29T23:46:44Z

mypy/messages.py

-        relatively short to avoid overly long error messages.
+    def format_bare(self, typ: Type, verbosity: int = 0) -> str:
+        """
+        Convert a type to a relatively short string suitable for error messages.


Again, an empty line after first line of docstring is needed here.

Same as before, there is a blank line it's just not considered added for whatever reason.

ilevkivskyi · 2017-08-29T23:54:47Z

mypy/messages.py

        Increase the verbosity of the type strings until they become distinct.
        """
+        if bare:
+            format_method = self.format_bare


Could you please add a comment why we need this more complex logic? Is it only for formatting callable arguments?

I've expanded the docstring.

ilevkivskyi · 2017-08-29T23:57:02Z

test-data/unit/check-async-await.test

    def __aexit__(self, x, y, z) -> int: pass
 async def f() -> None:
-    async with C() as x: # E: Incompatible types in "async with" for __aexit__ (actual type "int", expected type Awaitable[Any])
+    async with C() as x: # E: Incompatible types in "async with" for __aexit__ (actual type "int", expected type "Awaitable[Any]")


There is still some inconsistency in formatting attribute names here __aexit__ and few lines above "__await__". Would you like to fix this in a separate PR?

Yep, I suspect these will be more about specific message cleanup than anything else; I'll file an issue with the list of things you've spotted, so I don't lose track of them.

I've opened #3887 for this.

ilevkivskyi · 2017-08-29T23:58:04Z

test-data/unit/check-async-await.test

 async def plain_host_coroutine() -> None:
    x = 0
-    x = await plain_generator()  # E: Incompatible types in await (actual type Generator[str, None, int], expected type Awaitable[Any])
+    x = await plain_generator()  # E: Incompatible types in await (actual type "Generator[str, None, int]", expected type "Awaitable[Any]")


Similar situation here: await vs "yield from" few lines above.

But again, this is a story for a separate PR.

ilevkivskyi · 2017-08-30T00:07:07Z

test-data/unit/check-protocols.test

-apply_str(Add5(), 'a') # E: Argument 1 to "apply_str" has incompatible type "Add5"; expected Callable[[str], int] \
-                       # N: 'Add5.__call__' has type 'Callable[[Arg(int, 'x')], int]'
+apply_str(Add5(), 'a') # E: Argument 1 to "apply_str" has incompatible type "Add5"; expected "Callable[[str], int]" \
+                       # N: 'Add5.__call__' has type "Callable[[Arg(int, 'x')], int]"


Another potential idea: maybe use double quotes instead of single quotes in 'Add5.__call__'? (But not in this PR.)

ilevkivskyi · 2017-08-30T00:08:52Z

test-data/unit/check-tuples.test

 [builtins fixtures/tuple.pyi]
 [out]
-main:3: error: Unsupported operand types for + ("LongTypeName" and tuple(length 50))
+main:3: error: Unsupported operand types for + ("LongTypeName" and "tuple(length 50)")


I think tuple(length 50) should not be surrounded by quotes.

OddBloke · 2017-08-30T12:37:01Z

@ilevkivskyi So I'm looking at stripping quotes for union types, and the test I'm writing has this output now:

Argument 1 to "takes_int" has incompatible type "union type (6 items)"; expected "int"

but would have:

Argument 1 to "takes_int" has incompatible type union type (6 items); expected "int"

which I find quite hard to parse. (The tuple formatting doesn't have the same problem, because it doesn't repeat the word "type" (it's just tuple(length N)).

I wonder if we should leave the union representation in quotes and then perhaps improve it in a different PR? (I was thinking <union type: N items> and <tuple: length N> (or even <tuple: N items> if we want it to be consistent), which I think are easier to parse).

OddBloke · 2017-08-30T12:39:17Z

(I've pushed up a commit which does it just for tuples for now.)

ilevkivskyi · 2017-08-30T14:34:24Z

Actually I like the idea of <union: N items> and <tuple: N items>, but again it is better for a separate PR.

ilevkivskyi

Thanks! Looks good now.

OddBloke · 2017-08-30T14:51:10Z

\o/ Thanks!

Brian Weber and others added 17 commits May 23, 2017 15:01

Consistent quote wrapping around MessageBuilder.format as per issue p…

b4b7de9

…ython#3409

No quotes around Module

466a832

And even more tests that need to be updated

dc7c243

incorrect indentation

df58633

Consistent quote wrapping around MessageBuilder.format as per issue p…

ba55b1a

…ython#3409

No quotes around Module

e134f8f

And even more tests that need to be updated

60231de

incorrect indentation

735778e

update tests & merge conflicts

5922be3

and even more merge conflicts

b349b8e

Merge trunk into quotewrap

6a45cad

Fix trivial quoting mismatches

048dae1

Remove message-specific quoting from protocol error messages

3bba4bd

As all types are now quoted, this fixes places that they would be double-quoted in the protocol error messages.

Quote Callable types in message output

e6e1701

Update MessageBuilder.note_call to remove double-quoting of types

d884e03

Fix non-pytest test failures

e0b5a30

Merge remote-tracking branch 'upstream/master' into quotewrap

04393b6

OddBloke mentioned this pull request Aug 25, 2017

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

Closed

ilevkivskyi reviewed Aug 25, 2017

View reviewed changes

Daniel Watkins added 8 commits August 27, 2017 09:17

Move FunctionLike formatting from MessageBuilder.format to _format_si…

fc11a6f

…mple This makes the quoting code a little more consistent.

Move fallback formatting from MessageBuilder.format to _format_simple

f288a60

Remove MessageBuilder.format_simple

e10a803

Instead update MessageBuilder.format to call _format_simple directly.

Introduce MessageBuilder.format_bare

b4f0b78

This is ._format_simple renamed to reflect its new intended contract.

Use MessageBuilder.format_bare where appropriate

4bb8d3f

This replaces strip_quotes(self.format(...)) calls, which have identical effect.

Move * and ** inside quotes in MessageBuilder.incompatible_argument

8a27e2c

This adds a bare parameter to format_distinctly, so that MessageBuilder.incompatible_argument can take care of the quoting for its special case.

Merge remote-tracking branch 'upstream/master' into quotewrap

6a7ec9b

Fix long line

a8fe400

Fix up now-inaccurate comments

daa6792

ilevkivskyi reviewed Aug 30, 2017

View reviewed changes

Elaborate on the bare parameter in format_distinctly

96961c0

OddBloke mentioned this pull request Aug 30, 2017

Inconsistent attribute quoting in some error messages #3887

Closed

Don't quote <nothing> or <deleted> in messages

5bc7251

Don't surround tuple(length N) in quotes in message output

a84b4bb

ilevkivskyi approved these changes Aug 30, 2017

View reviewed changes

ilevkivskyi merged commit 3171c05 into python:master Aug 30, 2017

OddBloke deleted the quotewrap branch August 30, 2017 14:51

This was referenced Aug 30, 2017

Format many-itemed tuples and unions consistently and more clearly #3893

Merged

Incompatible return type error doesn't quote types #3771

Closed

Uh oh!

Conversation

OddBloke commented Aug 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OddBloke commented Aug 25, 2017 via email

Uh oh!

OddBloke commented Aug 26, 2017

Uh oh!

OddBloke commented Aug 27, 2017

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OddBloke commented Aug 30, 2017

Uh oh!

OddBloke commented Aug 30, 2017

Uh oh!

ilevkivskyi commented Aug 30, 2017

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

OddBloke commented Aug 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants