Skip to content
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

Manual changes of Any union to Incomplete in stubs folder #9566

Merged
merged 9 commits into from
Feb 2, 2023

Conversation

Avasam
Copy link
Sponsor Collaborator

@Avasam Avasam commented Jan 18, 2023

Follow up to #9565 . Ref #9550

  • ClassVar[Any | None]
  • Missed in previous automated changes due to Any being aliased
  • Manual review of leftover Any unions (| Any and Any |)

Avoid touching any return type (function or Callable). Or when a comment explained the reason behind using Any.

After this PR, all Any unions left can basically stay untouched. So we can avoid changing any of 'em, or return types, in the next stage.

- ClassVar[Any | None]
- Missed previous changes due to alias
- Manual review of leftover Any unions (`| Any` and `Any |`)
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Avasam Avasam changed the title Manual changes of Any to Incomplete in stubs folder Manual changes of Any union to Incomplete in stubs folder Jan 19, 2023
def dump_inline_table(self, section: dict[str, Any] | Any) -> str: ...
def dump_value(self, v: Any) -> str: ...
def dump_inline_table(self, section: object) -> str: ...
def dump_value(self, v: object) -> str: ...
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this should be object? I don't think toml can dump all objects.

Copy link
Sponsor Collaborator Author

@Avasam Avasam Feb 1, 2023

Choose a reason for hiding this comment

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

From what I can see of the implementation, and from trying it, if it doesn't specifically handle an object it just stringifies it.

>>> TomlEncoder().dump_inline_table(object()) 
'"<object object at 0x000001FF99FF85A0>"'
>>> TomlEncoder().dump_value(object())        
'"<object object at 0x000001FF99FF85A0>"'
>>> TomlEncoder().dump_value(dict())   
'[]'
>>> TomlEncoder().dump_value([object()]) 
'[ "<object object at 0x000001FF99FF85A0>",]'
>>> TomlEncoder().dump_inline_table([object()]) 
'[ "<object object at 0x000001FF99FF85B0>",]'
>>> TomlEncoder().dump_inline_table({"aaa": object()}) 
'{ aaa = "<object object at 0x000001FF99FF85A0>" }\n'
>>> TomlEncoder().dump_value({"aaa": object()})        
'[ "aaa",]'

Copy link
Collaborator

Choose a reason for hiding this comment

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

While technically it can handle all objects, in that it doesn't throw an exception, not all objects are correct arguments. "<object object at 0x000001FF99FF85B0>" is obviously not useful as an output.

Copy link
Sponsor Collaborator Author

@Avasam Avasam Feb 1, 2023

Choose a reason for hiding this comment

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

And trying to represent intended values, we kindof hit the same issue as JSON.
There's also classes with custom repr that can be dumped.
And subclassed Encoder that extend dump_funcs to support more intended types.

To be consistent though, how about the dump_list method typed with Iterable[object] ? I feel the same logic should apply and be Iterable[Any]. (it's calling dump_value internally)

Copy link
Member

Choose a reason for hiding this comment

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

That's why Any makes sense here: the type annotation we'd ideally want isn't representable in the type system.

from typing import Any, ClassVar

from yaml.error import Mark

class Node:
tag: str
value: Any
start_mark: Mark | Any
end_mark: Mark | Any
start_mark: Mark | Incomplete
Copy link
Member

Choose a reason for hiding this comment

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

This kind of looks like it's using the | Any trick for when the type is almost always one particular type (like we do for re.match). If so, Any is right.

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

Those attributes are directly assigned from the parameters, so they should be the same.
If it's possible, and intended to be anything else than Mark | None, with an arbitrary type, or if it's to avoid having to check for a None mark when you know you instantiated it not-None. And that's a common use-case/pattern. That can be represented using generics. Since it's feasible within the current typing system, Incomplete (could be done but we didn't bothered) seems preferable over Any (can't be done, we tried)

Copy link
Sponsor Collaborator Author

@Avasam Avasam Feb 1, 2023

Choose a reason for hiding this comment

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

I think there's an argument to be made here for either permissive unions or default generics. Which both aren't part of the type system atm. I've changed the type to Mark | None | AnyMark | Any and added a comment to explain.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

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

@JelleZijlstra JelleZijlstra merged commit f1aede7 into python:main Feb 2, 2023
@Avasam Avasam deleted the manual-Any-Union branch February 2, 2023 15: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.

3 participants