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

[filbranden]

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.

[Hnasar]

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

[filbranden]

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?

[Hnasar]

Thanks for starting on a fix!

[hynek]

Thanks for sorting it out yourself y'all! Feel free to open a PR re the typing issue you mentioned.

[filbranden]

Thanks for merging @hynek !

I have a follow up on typing on #1373 but looks like that's still failing CI, looks like mypy on 3.10+ breaks with the error in the comments (even though tox -e py310-mypy itself doesn't fail), I'll take another look later see if I can figure out why it's breaking.

I also posted #1374 to fix another issue I uncovered in converters, in a situation where they evaluate to False. Long story short, but doing an explicit comparison to None fixes that and I believe it makes sense here.

Cheers!