-
Notifications
You must be signed in to change notification settings - Fork 78
Delete unnecessary comparison operators #698
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
base: v1
Are you sure you want to change the base?
Conversation
Not sure what's going on with the CI failures 🤔 |
Also not sure, looks like something has broken PyCall (a test dependency) in CI. Unrelated to this though. |
This is a tricky one because on the one hand, these definitions do need to stay as they are because Python is weird. For example On the other hand, these comparison operators return a One can view this latter point as a bug and therefore fixable in a technically non-breaking fashion. But it definitely will break people's code. We're working (extremely slowly) towards PythonCall v1 which can definitely fix these issues. The right thing to do would be to replace all these methods with ones that correctly return |
Thinking some more, actually I think these methods are not documented except there is one example which implies that So perhaps it's ok to change these methods in a non-breaking release since the behaviour is (almost) undocumented, and is incompatible with the definition of However like I said, this change will quite likely break a bunch of code in practice. |
Mmmm, interesting. I feel like it's probably safer to put this in a breaking release then. |
I've just changed the target branch from main to v1 but now this PR has a bunch of commits on main that aren't merged into v1 yet. I'll do that later. |
These are already defined by Base in terms of other comparison operators, so they cause a ton of invalidations.
efab5e5
to
7cc0f45
Compare
I rebased it :) Do you know when v1 (or an RC) will be released? |
Thanks. I'm not sure but there's a milestone to track it: https://github.com/JuliaPy/PythonCall.jl/milestone/1 It's been in the works for a very long time but I'm actually managing to chip away at it at the moment. There are two major PRs to do, one of which is basically ready and one is unstarted but the thinking has been done. Then there are a bunch of little things. |
These are already defined by Base in terms of other comparison operators, so they cause a ton of invalidations. I believe the Python operators have the same logical requirements as the corresponding Julia ones, so this should still be correct 🤞
The total number of invalidations on Julia 1.12 went down to 8551 from 11647 (not including the fixed invalidations from the UnsafePointers.jl and Julia PRs, those remove a few more thousand). These invalidations should be fixed: