yaml.dump(..., stream) is optional #2289

pmhahn · 2018-06-28T06:39:15Z

http://pyyaml.org/wiki/PyYAMLDocumentation#dumping-yaml

yaml.dump accepts the second optional argument, which must be an open
text or binary file. In this case, yaml.dump will write the produced
YAML document into the file. Otherwise, yaml.dump returns the produced
document.

v2: Also update the safe_dump[_all] functions and extend the description to state that stream=None makes the function return the result instead of writing it into the stream to address #2288 (comment)

JelleZijlstra · 2018-06-28T06:50:54Z

Does that mean we could get a more precise type with an overload that says that the functions return None if the stream argument is an IO, and a str if it's None or not given?

pmhahn · 2018-06-28T07:43:57Z

overload is new for me and took me some to to get the order right, but now it works for me:

#!/usr/bin/python2.7
from yaml import load, dump
def main():  # type: () -> None
    doc = load("a: 1")
    with open('/dev/null', 'w') as stream:
        dump(doc, stream)
    out = dump(doc, None)
    print(out)
if __name__ == '__main__':
  main()

pmhahn · 2018-06-28T08:20:32Z

No, it still does not work. I'm still getting

/usr/lib/mypy/typeshed/third_party/2and3/yaml/init.pyi:27: error: Overloaded function signatures 1 and 2 overlap with incompatible return types
/usr/lib/mypy/typeshed/third_party/2and3/yaml/init.pyi:32: error: Overloaded function signatures 1 and 2 overlap with incompatible return types
/usr/lib/mypy/typeshed/third_party/2and3/yaml/init.pyi:37: error: Overloaded function signatures 1 and 2 overlap with incompatible return types
/usr/lib/mypy/typeshed/third_party/2and3/yaml/init.pyi:42: error: Overloaded function signatures 1 and 2 overlap with incompatible return types

After reading python/mypy#1941 I had hoped to get it right.

ilevkivskyi · 2018-06-28T09:37:41Z

third_party/2and3/yaml/__init__.pyi

+@overload
+def dump(data: Any, stream: None=..., Dumper=..., **kwds) -> AnyStr: ...
+@overload
+def dump(data: Any, stream: IO[str]=..., Dumper=..., **kwds) -> None: ...


I think you don't need =... in the second overload here and in other functions, otherwise there is clearly an overlap, because dump(something) matches both.

Yes, that's it: After removing the =... it seems to work. Will --amend and push --force

ilevkivskyi · 2018-06-28T09:38:24Z

third_party/2and3/yaml/__init__.pyi

+def dump_all(documents: Sequence[Any], stream: IO[str]=..., Dumper=..., default_style=..., default_flow_style=..., canonical=..., indent=..., width=..., allow_unicode=..., line_break=..., encoding=..., explicit_start=..., explicit_end=..., version=..., tags=...) -> None: ...
+
+@overload
+def dump(data: Any, stream: None=..., Dumper=..., **kwds) -> AnyStr: ...


Shouldn't the return be just str here and in other functions?

No, read http://pyyaml.org/wiki/PyYAMLDocumentation#python-3-support:

from yaml import dump print(type(dump('', None))) # str print(type(dump('', None, encoding=None))) # unicode print(type(dump('', None, encoding='utf-8'))) # str

Hm, in that case Union[str, bytes] may be better. Although I know union return types are strongly discouraged. But the current way of using AnyStr as a "hackish" way to implement an unsafe union is purely based on how mypy behaves, it is not specified in PEP 484, so can break at any moment. @JelleZijlstra is this a common practice?

I didn't even know this worked like an unsafe union; I've been consistently telling people not to use AnyStr in a single position in a signature.

Regardless, can't this also be fixed with more overloads? It returns unicode if encoding is None or omitted and str when it's a string.

I tried it with this:

+@overload +def serialize_all(nodes, stream: None=..., Dumper=..., canonical=..., indent=..., width=..., allow_unicode=..., line_break=..., encoding: None=..., explicit_start=..., explicit_end=..., version=..., tags=...) -> Text: ... +@overload +def serialize_all(nodes, stream: None=..., Dumper=..., canonical=..., indent=..., width=..., allow_unicode=..., line_break=..., encoding: str, explicit_start=..., explicit_end=..., version=..., tags=...) -> bytes: ... +@overload +def serialize_all(nodes, stream: IO[str], Dumper=..., canonical=..., indent=..., width=..., allow_unicode=..., line_break=..., encoding=..., explicit_start=..., explicit_end=..., version=..., tags=...) -> None: ...

But mypy complain about this:

third_party/2and3/yaml/__init__.pyi:27: error: non-default argument follows default argument

For reference:

a = dump('') # type: str b = dump('', None) # type: str c = dump('', None, encoding=None) # type: Text # 2:unicode 3:str d = dump('', None, encoding='utf-8') # type: bytes # 2:str 3:bytes

You can just add # type: ignore if reveal_type() shows all correct types all call sites.

@ilevkivskyi Your comment confuses me; the #type annotation in my last comment is only to document the type returned by dump(); that code snippet makes it easy to validate the yaml/__init__.pyi file by running mypy --custom-typeshed-dir=. snipped.py with and without --py2 and getting an error message.

So now what:

get the @overload working to return the correct type depending on encoding?

replace the AnyStr by what?

split the 2and3 into two versions to get rid of Text being unicode with Python2 and str with Python3?

something other?

Add # type: ignore to silence the error you see in stubs (non-default argument follows default argument). And then check if this leads to correct results in user code by using reveal_type(), this is a mypy trick reveal_type(<expression>) creates an error that shows the type of <expression> from mypy's point of view.

JelleZijlstra · 2018-06-28T15:23:44Z

@pmhahn no need to force-push and amend; it actually makes review harder sometimes. It's fine to just push new commits to this PR; we'll clean up the commit message before merging.

JelleZijlstra · 2018-06-28T15:30:26Z

third_party/2and3/yaml/__init__.pyi

+@overload
+def safe_dump_all(documents: Sequence[Any], stream: None, **kwds) -> AnyStr: ...
+@overload
+def safe_dump_all(documents: Sequence[Any], stream: IO[str]=..., **kwds) -> None: ...


The =... should go to the other overload; it returns a string, not None, if you omit the stream argument.

Fixed by a78cca2

third_party/2and3/yaml/__init__.pyi

srittau · 2018-10-21T12:07:50Z

@pmhahn Are you still interested in this pull request?

pmhahn · 2018-10-25T13:35:19Z

I had to rebase the series because of an conflict with 006a792
I verified both python2.7 and python3.5 to show now errors.
I hope all issues are now resolved.

JelleZijlstra · 2018-10-26T04:04:24Z

third_party/2and3/yaml/__init__.pyi

+@overload
+def serialize_all(nodes, stream: None=..., Dumper=..., canonical=..., indent=..., width=..., allow_unicode=..., line_break=..., encoding: None=..., explicit_start=..., explicit_end=..., version=..., tags=...) -> str: ...
+@overload
+def serialize_all(nodes, stream: None=..., Dumper=..., canonical=..., indent=..., width=..., allow_unicode=..., line_break=..., encoding: str=..., explicit_start=..., explicit_end=..., version=..., tags=...) -> bytes: ...


Mypy complains about this because both overloads have = ... for the encoding argument. One of the two overloads should have no default for encoding.

Until python/mypy#5621 is fixed I see no practical way to resolve this: encoding is in the middle of 11 optional parameters and using that workaround would require 2^6 overload variants of those 6 functions.
I'm out of ideas for now.

JelleZijlstra · 2018-10-26T04:04:45Z

Travis shows a few issues still; could you fix those? (See also the comment I just added.)

srittau · 2018-12-02T12:46:18Z

CI fails due to a missing import and linter issues. Also, there is a conflict now.

as such move the `=...` to the None-case.

same as for dump_all()

@overload

Depending on if a "stream" or the "encoding" is given, the functions either return None, str/unicode or bytes. Use @overload fix distinguish those cases. Also fix the functions using **kwds as they delegate their work to the more generic functions: copy their signatures.

As far as I know it's currently impossible to use "overloads with return types depending on optional arguments" (Issue python#5621). As "encoding" is in the middle of 11 optional arguments, it would require ~ 2^6 overloads for each of those 6 functions. For now just return Any and wait for Issue python#5621 to get fixed.

pmhahn · 2018-12-04T12:14:05Z

I added the missing imports.
I rebased my branch on master and force-pushed it. (or do you prefer I do a merge?)

flake8 E252

pmhahn · 2018-12-05T14:23:15Z

CI fails due to a missing import and linter issues. Also, there is a conflict now.

The flake8 issues should now be fixed with e5c943c - what a pain to get the new flake8 version in Debian-Stretch as it only has Python3.5 and virtualenv is currently broken in Debian-Sid

outdated

pmhahn mentioned this pull request Jun 28, 2018

yaml.dump(..., stream) is optional #2288

Closed

pmhahn force-pushed the yaml branch from 0ecab10 to 0ee0f58 Compare June 28, 2018 07:40

ilevkivskyi reviewed Jun 28, 2018

View reviewed changes

pmhahn force-pushed the yaml branch from 0ee0f58 to 46fb62c Compare June 28, 2018 12:23

JelleZijlstra reviewed Jun 28, 2018

View reviewed changes

JelleZijlstra previously requested changes Jul 3, 2018

View reviewed changes

third_party/2and3/yaml/__init__.pyi Outdated Show resolved Hide resolved

pmhahn force-pushed the yaml branch from b887713 to 415cca0 Compare October 25, 2018 13:33

JelleZijlstra reviewed Oct 26, 2018

View reviewed changes

pmhahn added 4 commits December 4, 2018 13:02

yaml.dump(..., stream) is optional

8a453d0

as such move the `=...` to the None-case.

yaml.serialize_all(..., stream) overload

b4617ca

same as for dump_all()

pmhahn force-pushed the yaml branch from 77c5c8d to c7a8606 Compare December 4, 2018 12:08

Add missing whitespace around parameter equals

e5c943c

flake8 E252

Merge branch 'master' into yaml

b37c425

JelleZijlstra self-requested a review May 5, 2019 19:17

clarify what issue is being referred to

64bec4a

JelleZijlstra merged commit 548dbbb into python:master May 6, 2019

pmhahn deleted the yaml branch May 7, 2019 05:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

yaml.dump(..., stream) is optional #2289

yaml.dump(..., stream) is optional #2289

pmhahn commented Jun 28, 2018

JelleZijlstra commented Jun 28, 2018

pmhahn commented Jun 28, 2018

pmhahn commented Jun 28, 2018 •

edited

Loading

ilevkivskyi Jun 28, 2018

pmhahn Jun 28, 2018

ilevkivskyi Jun 28, 2018

pmhahn Jun 28, 2018

ilevkivskyi Jun 28, 2018

JelleZijlstra Jun 28, 2018

pmhahn Jun 29, 2018

ilevkivskyi Jun 29, 2018

pmhahn Jun 29, 2018

ilevkivskyi Jun 29, 2018 •

edited

Loading

JelleZijlstra commented Jun 28, 2018

JelleZijlstra Jun 28, 2018

pmhahn Jun 29, 2018

srittau commented Oct 21, 2018

pmhahn commented Oct 25, 2018

JelleZijlstra Oct 26, 2018

pmhahn Oct 28, 2018 •

edited

Loading

JelleZijlstra commented Oct 26, 2018

srittau commented Dec 2, 2018

pmhahn commented Dec 4, 2018 •

edited

Loading

pmhahn commented Dec 5, 2018

yaml.dump(..., stream) is optional #2289

yaml.dump(..., stream) is optional #2289

Conversation

pmhahn commented Jun 28, 2018

JelleZijlstra commented Jun 28, 2018

pmhahn commented Jun 28, 2018

pmhahn commented Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

JelleZijlstra commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srittau commented Oct 21, 2018

pmhahn commented Oct 25, 2018

Choose a reason for hiding this comment

pmhahn Oct 28, 2018 • edited Loading

Choose a reason for hiding this comment

JelleZijlstra commented Oct 26, 2018

srittau commented Dec 2, 2018

pmhahn commented Dec 4, 2018 • edited Loading

pmhahn commented Dec 5, 2018

pmhahn commented Jun 28, 2018 •

edited

Loading

ilevkivskyi Jun 29, 2018 •

edited

Loading

pmhahn Oct 28, 2018 •

edited

Loading

pmhahn commented Dec 4, 2018 •

edited

Loading