Skip to content

PyYAML: Permit width: float for pure-Python dump(...) #8973

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

Merged
merged 3 commits into from
Nov 8, 2022

Conversation

kmurphy4
Copy link
Contributor

To prevent PyYAML from wrapping any lines, it's possible to pass width=float("inf"), but the current type hints don't like that. This works at runtime:

>>> s = yaml.dump({"foo": "bar" * 1000}, width=float("inf"))
>>> print(s)
foo: barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar...

but mypy says

floatwidth.py:2: error: No overload variant of "dump" matches argument types "Dict[str, str]", "float"
floatwidth.py:2: note: Possible overload variants:
floatwidth.py:2: note:     def dump(data: Any, stream: _WriteStream[Any], Dumper: Any = ..., *, default_style: Optional[str] = ..., default_flow_style: Optional[bool] = ..., canonical: Optional[bool] = ..., indent: Optional[int] = ..., width: Optional[int] = ..., allow_unicode: Optional[bool] = ..., line_break: Optional[str] = ..., encoding: Optional[str] = ..., explicit_start: Optional[bool] = ..., explicit_end: Optional[bool] = ..., version: Optional[Tuple[int, int]] = ..., tags: Optional[Mapping[str, str]] = ..., sort_keys: bool = ...) -> None
floatwidth.py:2: note:     def dump(data: Any, stream: None = ..., Dumper: Any = ..., *, default_style: Optional[str] = ..., default_flow_style: Optional[bool] = ..., canonical: Optional[bool] = ..., indent: Optional[int] = ..., width: Optional[int] = ..., allow_unicode: Optional[bool] = ..., line_break: Optional[str] = ..., encoding: Optional[str] = ..., explicit_start: Optional[bool] = ..., explicit_end: Optional[bool] = ..., version: Optional[Tuple[int, int]] = ..., tags: Optional[Mapping[str, str]] = ..., sort_keys: bool = ...) -> Any
Found 1 error in 1 file (checked 1 source file)

Poking through the PyYAML source, it looks the width parameter could probably be anything "comparable", as it's only compared via the < operator.

For the LibYAML implementation, however, we have to use ints:

>>> stream = io.StringIO()
>>> dumper = yaml.CDumper(stream, width=float("inf"))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3/dist-packages/yaml/cyaml.py", line 81, in __init__
    version=version, tags=tags)
  File "ext/_yaml.pyx", line 973, in _yaml.CEmitter.__init__ (ext/_yaml.c:14797)
OverflowError: cannot convert float infinity to integer

I'm not familiar with Cython, though I assume this is because of the definition of yaml_emitter_set_width.

To prevent `PyYAML` from wrapping *any* lines, it's possible to pass
`width=float("inf")`, but the current type hints don't like that.  This
works at runtime:

    >>> s = yaml.dump({"foo": "bar" * 1000}, width=float("inf"))
    >>> print(s)
    foo: barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar...

but `mypy` says

    floatwidth.py:2: error: No overload variant of "dump" matches argument types "Dict[str, str]", "float"
    floatwidth.py:2: note: Possible overload variants:
    floatwidth.py:2: note:     def dump(data: Any, stream: _WriteStream[Any], Dumper: Any = ..., *, default_style: Optional[str] = ..., default_flow_style: Optional[bool] = ..., canonical: Optional[bool] = ..., indent: Optional[int] = ..., width: Optional[int] = ..., allow_unicode: Optional[bool] = ..., line_break: Optional[str] = ..., encoding: Optional[str] = ..., explicit_start: Optional[bool] = ..., explicit_end: Optional[bool] = ..., version: Optional[Tuple[int, int]] = ..., tags: Optional[Mapping[str, str]] = ..., sort_keys: bool = ...) -> None
    floatwidth.py:2: note:     def dump(data: Any, stream: None = ..., Dumper: Any = ..., *, default_style: Optional[str] = ..., default_flow_style: Optional[bool] = ..., canonical: Optional[bool] = ..., indent: Optional[int] = ..., width: Optional[int] = ..., allow_unicode: Optional[bool] = ..., line_break: Optional[str] = ..., encoding: Optional[str] = ..., explicit_start: Optional[bool] = ..., explicit_end: Optional[bool] = ..., version: Optional[Tuple[int, int]] = ..., tags: Optional[Mapping[str, str]] = ..., sort_keys: bool = ...) -> Any
    Found 1 error in 1 file (checked 1 source file)

Poking through the `PyYAML` source, it looks the `width` parameter
could probably be anything "comparable", as it's only compared via
the `<` operator[1].

For the LibYAML implementation, however, we have to use `int`s:

    >>> stream = StringIO()
    >>> dumper = yaml.CDumper(stream, width=float("inf"))
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/usr/lib/python3/dist-packages/yaml/cyaml.py", line 81, in __init__
        version=version, tags=tags)
      File "ext/_yaml.pyx", line 973, in _yaml.CEmitter.__init__ (ext/_yaml.c:14797)
    OverflowError: cannot convert float infinity to integer

I'm not familiar with Cython, though I assume this is because of
the definition of `yaml_emitter_set_width`[2].

[1] https://github.com/yaml/pyyaml/blob/8cdff2c80573b8be8e8ad28929264a913a63aa33/lib/yaml/emitter.py#L89
[2] https://github.com/yaml/pyyaml/blob/8cdff2c80573b8be8e8ad28929264a913a63aa33/yaml/_yaml.pxd#L252
Apparently `flake8` doesn't like `int | float`[1], though I personally
think this is a bit less clear... 🤷

[1] https://github.com/python/typeshed/actions/runs/3308882007/jobs/5461582110
@github-actions

This comment has been minimized.

@Avasam
Copy link
Collaborator

Avasam commented Nov 7, 2022

Ref python/typing#1160

srittau
srittau previously requested changes Nov 7, 2022
Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

I'm sorry for the late response. The change looks good functionally, but i would prefer if we would define a type alias _Inf = float or similar (with an explanatory comment) and then use int | _Inf | None. While functionally identically, it makes the intent clearer and would make it easier to potentially change it to Literal['inf'] (or whatever) in the future.

@kmurphy4
Copy link
Contributor Author

kmurphy4 commented Nov 7, 2022

No worries, thanks for the review :)

I'm happy to add the type alias, but flake8 complains about int | float (see the last commit message).

@AlexWaygood
Copy link
Member

flake8-pyi is pretty dumb about these things — it'll detect int | float and yell at you, but if you do _Inf: TypeAlias = float, it'll be absolutely cool with you using int | _Inf :)

kmurphy4 added a commit to kmurphy4/typeshed that referenced this pull request Nov 7, 2022
@kmurphy4 kmurphy4 force-pushed the pyyaml/permit-float-widths branch from 2549f71 to 906ca9e Compare November 7, 2022 15:32
@github-actions
Copy link
Contributor

github-actions bot commented Nov 7, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood AlexWaygood dismissed srittau’s stale review November 8, 2022 21:22

The requested changes were made

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

@AlexWaygood AlexWaygood merged commit 0884fe1 into python:main Nov 8, 2022
@kmurphy4 kmurphy4 deleted the pyyaml/permit-float-widths branch November 8, 2022 21:24
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.

4 participants