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

Make attrs.converters.pipe only return a Converter instance if one is passed #1380

Merged
merged 3 commits into from
Dec 10, 2024

Conversation

filbranden
Copy link
Contributor

Summary

Make pipe() return a regular callable if only passed callables, only return a Converter instance if one is strictly required (by having a Converter instance within the pipeline.)

Some tests are affected by this change (mostly in how __annotations__ is accessed), this PR reverts most of them to their state before #1267.

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.
  • (N/A) 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
Copy link
Contributor Author

@hynek @Tinche please take a look, let me know if you have any questions or would like to see any changes in this PR.

Also, I'd appreciate it if this and my previous couple of PRs could make it into an official release soon, is there a chance you'd consider cutting a 24.3.0 after this PR gets merged? Thanks!

@hynek
Copy link
Member

hynek commented Dec 9, 2024

@hynek @Tinche please take a look, let me know if you have any questions or would like to see any changes in this PR.

Would you mind sharing the reasoning? It was kinda on purpose to normalize how the converters look like but I'm not married to it.

Also, I'd appreciate it if this and my previous couple of PRs could make it into an official release soon, is there a chance you'd consider cutting a 24.3.0 after this PR gets merged? Thanks!

I will do my best to release a 24.3 before NYE. We need to fix #1347 first, hopefully merge #1358, and ideally implement #1313 (but that's more of a hope than a requirement).

@filbranden
Copy link
Contributor Author

Would you mind sharing the reasoning? It was kinda on purpose to normalize how the converters look like but I'm not married to it.

Sure. In trying to upgrade to 24.2.0, I found some situations where code was calling one of the converter functions and trying to use that directly as a callable (I believe it was with optional(...) rather than pipe(...), but I'm not 100% sure there weren't cases of using pipe() that way. When I pushed #1372 I made sure the updated optional(...) would return a callable if passed a callable.)

The other issue I've encountered is that Converter instances trigger a bug in inspect.signature() in Python 3.10. This has been fixed in python/cpython#101293 but this was never backported to Python 3.10. I found other workarounds that involve adding a __call__() to Converter, or setting __signature__ explicitly if detecting Python 3.10 and under, but it turns out returning a callable when passed a callable seems to address my use cases of that as well, so it doesn't require any other workarounds anymore.

I will do my best to release a 24.3 before NYE.

Thank you!

@filbranden
Copy link
Contributor Author

Updated to add type signatures with overloads, that specify passing callables will return a callable for both optional() (already the case) and pipe() (changed in this PR.) Plus a few other minor cleanups.

Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

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

thanks!

If you come up with a pithy changelog entry, ideally spinning it as a feature for our users, feel free to drop it here or open a quick PR.

@hynek hynek added this pull request to the merge queue Dec 10, 2024
Merged via the queue into python-attrs:main with commit 1e07f46 Dec 10, 2024
19 checks passed
@filbranden filbranden deleted the converterpipe1 branch December 10, 2024 18:14
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.

2 participants