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

Replace __new__ componentwise #247

Merged
merged 3 commits into from
Nov 25, 2024
Merged

Conversation

alexfikl
Copy link
Collaborator

@alexfikl alexfikl commented Nov 16, 2024

Fixes #246.

@alexfikl alexfikl marked this pull request as ready for review November 17, 2024 19:13
@inducer
Copy link
Owner

inducer commented Nov 17, 2024

Thanks! One thing that went through my head is whether we should outright deprecate direct constructor calls that bypass the factory functions.

@inducer
Copy link
Owner

inducer commented Nov 17, 2024

Implied but not said: how do you feel about that idea?

@alexfikl
Copy link
Collaborator Author

Thanks! One thing that went through my head is whether we should outright deprecate direct constructor calls that bypass the factory functions.

Calling the constructor directly should still be ok for "internal" code though, right? e.g. all the mappers call the constructor directly.

@alexfikl alexfikl force-pushed the fix-componentwise branch 2 times, most recently from c7a184e to 9f471cf Compare November 18, 2024 08:03
@inducer
Copy link
Owner

inducer commented Nov 18, 2024

Calling the constructor directly should still be ok for "internal" code though, right? e.g. all the mappers call the constructor directly.

Yes, definitely. One question is whether we'll make them pass a flag to say, "I'm internal!"

@alexfikl
Copy link
Collaborator Author

Yes, definitely. One question is whether we'll make them pass a flag to say, "I'm internal!"

I don't know, I agree that we should discourage that somehow, but maybe a little blurb in the docs will do the trick? I'm mostly hesitant because: adding a little flag is annoying + none of the other expressions have a deprecated constructor.

@inducer inducer merged commit 223ab83 into inducer:main Nov 25, 2024
9 checks passed
@inducer
Copy link
Owner

inducer commented Nov 25, 2024

This is awesome. Thanks so much for doing this!

@alexfikl alexfikl deleted the fix-componentwise branch November 25, 2024 06:18
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.

Discussion: Deprecate overloading __new__ for operator application?
2 participants