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

Allow converter.optional to take a Converter such as converter.pipe as its argument #1372

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

filbranden
Copy link

@filbranden filbranden commented Nov 13, 2024

Summary

Allow converter.optional to take a Converter such as converter.pipe as its argument.

Fixes #1348

Pull Request Check List

  • Do not open pull requests from your main branch – use a separate branch!
    • There's a ton of footguns waiting if you don't heed this warning. You can still go back to your project, create a branch from your main branch, push it, and open the pull request from the new branch.
    • This is not a pre-requisite for your pull request to be accepted, but you have been warned.
  • Added tests for changed code.
    Our CI fails if coverage is not 100%.
  • (N/A) New features have been added to our Hypothesis testing strategy.
  • (N/A) Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
  • (N/A) Updated documentation for changed code.
  • Documentation in .rst and .md files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.
  • Consider granting push permissions to the PR branch, so maintainers can fix minor issues themselves without pestering you.

@filbranden filbranden changed the title Allow converter.optional to take a converter such as converter.pipe as its argument Allow converter.optional to take a Converter such as converter.pipe as its argument Nov 13, 2024
def optional_converter(val, inst, field):
if val is None:
return None
return converter(val, inst, field)
Copy link

Choose a reason for hiding this comment

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

It's not guaranteed that the converter takes 3 arguments.

The root issue is that earlier versions of attrs had a "basic" converter that just took a single val. But now with the Converter class, Converter callables may take one, two (self), two(field), or three parameters.

Some introspection here may be necessary.

Also, since converters now may be callables with one two or three params, the type signature ought to be updated as well

https://github.com/python-attrs/attrs/blob/main/src/attrs/__init__.pyi#L54

Something like Callable[[Any], Any] | Callable[[Any, Any], Any] | Callable[[Any, Any, Any], Any]?

Copy link
Author

Choose a reason for hiding this comment

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

Actually a Converter instance always takes three arguments:

attrs/src/attr/_make.py

Lines 2712 to 2725 in 13e9a6a

if not (self.takes_self or self.takes_field):
self.__call__ = lambda value, _, __: self.converter(value)
elif self.takes_self and not self.takes_field:
self.__call__ = lambda value, instance, __: self.converter(
value, instance
)
elif not self.takes_self and self.takes_field:
self.__call__ = lambda value, __, field: self.converter(
value, field
)
else:
self.__call__ = lambda value, instance, field: self.converter(
value, instance, field
)

The callable it takes as an argument can take one, two or three, but the Converter class normalizes it to always take three arguments

So I believe this is correct

I think the typing information might need an update, to accept Callable[[Any], Any] | Converter but I think that's a separate issue so maybe we should follow up on that on a separate PR?

Copy link

@Hnasar Hnasar left a comment

Choose a reason for hiding this comment

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

Thanks for starting on a fix!

The constructor consumes __annotations__, so move the constructor call to after those have been set on the optional_converter function
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.

Error occurs when using converters.optional and converters.pipe together
2 participants