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

Consistent quote wrapping around MessageBuilder.format #3873

Merged
merged 29 commits into from Aug 30, 2017

Conversation

Projects
None yet
3 participants
@OddBloke
Contributor

OddBloke commented Aug 25, 2017

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

@OddBloke

This comment has been minimized.

Show comment
Hide comment
@OddBloke

OddBloke Aug 25, 2017

Contributor
Contributor

OddBloke commented Aug 25, 2017

@OddBloke

This comment has been minimized.

Show comment
Hide comment
@OddBloke

OddBloke Aug 26, 2017

Contributor

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.

Contributor

OddBloke commented Aug 26, 2017

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.

OddBloke added some commits Aug 27, 2017

Move FunctionLike formatting from MessageBuilder.format to _format_si…
…mple

This makes the quoting code a little more consistent.
Remove MessageBuilder.format_simple
Instead update MessageBuilder.format to call _format_simple directly.
Introduce MessageBuilder.format_bare
This is ._format_simple renamed to reflect its new intended contract.
Use MessageBuilder.format_bare where appropriate
This replaces strip_quotes(self.format(...)) calls, which have identical
effect.
Move * and ** inside quotes in MessageBuilder.incompatible_argument
This adds a bare parameter to format_distinctly, so that
MessageBuilder.incompatible_argument can take care of the quoting for
its special case.
@OddBloke

This comment has been minimized.

Show comment
Hide comment
@OddBloke

OddBloke Aug 27, 2017

Contributor

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

Contributor

OddBloke commented Aug 27, 2017

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

@ilevkivskyi

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

Show outdated Hide outdated mypy/messages.py
Show outdated Hide outdated mypy/messages.py
Show outdated Hide outdated mypy/messages.py
Show outdated Hide outdated mypy/messages.py
@@ -312,7 +312,7 @@ class C:
async def __aenter__(self) -> int: pass
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]")

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 30, 2017

Collaborator

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?

@ilevkivskyi

ilevkivskyi Aug 30, 2017

Collaborator

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?

This comment has been minimized.

@OddBloke

OddBloke Aug 30, 2017

Contributor

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.

@OddBloke

OddBloke Aug 30, 2017

Contributor

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.

This comment has been minimized.

@OddBloke

OddBloke Aug 30, 2017

Contributor

I've opened #3887 for this.

@OddBloke

OddBloke Aug 30, 2017

Contributor

I've opened #3887 for this.

x = yield from other_iterator()
x = yield from other_coroutine() # E: "yield from" can't be applied to "Aw"
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]")

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 30, 2017

Collaborator

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

@ilevkivskyi

ilevkivskyi Aug 30, 2017

Collaborator

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 30, 2017

Collaborator

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

@ilevkivskyi

ilevkivskyi Aug 30, 2017

Collaborator

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

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]"

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Aug 30, 2017

Collaborator

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

@ilevkivskyi

ilevkivskyi Aug 30, 2017

Collaborator

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

Show outdated Hide outdated test-data/unit/check-tuples.test
@OddBloke

This comment has been minimized.

Show comment
Hide comment
@OddBloke

OddBloke Aug 30, 2017

Contributor

@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).

Contributor

OddBloke commented Aug 30, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@OddBloke

OddBloke Aug 30, 2017

Contributor

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

Contributor

OddBloke commented Aug 30, 2017

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

@ilevkivskyi

This comment has been minimized.

Show comment
Hide comment
@ilevkivskyi

ilevkivskyi Aug 30, 2017

Collaborator

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

Collaborator

ilevkivskyi commented Aug 30, 2017

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.

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@OddBloke OddBloke deleted the OddBloke:quotewrap branch Aug 30, 2017

@OddBloke

This comment has been minimized.

Show comment
Hide comment
@OddBloke

OddBloke Aug 30, 2017

Contributor

\o/ Thanks!

Contributor

OddBloke commented Aug 30, 2017

\o/ Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment