Skip to content

Conversation

@rvsampson94
Copy link
Contributor

Addresses part of issue #2898.
After this and pull request #3421 are merged, issue #2898 will be resolved.

@Snowapril
Copy link
Contributor

This seems work properly as isdisjoint method at now.
though, to_set is quite expensive method as it makes a redundant copy.
After #3316 merged, we need to modify it with PySequence_Contains like cpython implementation 😊

youknowone
youknowone previously approved these changes Nov 15, 2021
@youknowone youknowone dismissed their stale review November 15, 2021 16:24

tests fails

@rvsampson94
Copy link
Contributor Author

I will get the tests passing and resubmit. @Snowapril would you like me to add a TODO to come back and update this implementation after #3316 is merged?

@Snowapril
Copy link
Contributor

@rvsampson94 Oh yes of course. but rather than that, you could add TODO comment in isdisjoint. maybe it is better

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

looks great, thank you!

@youknowone youknowone merged commit 0fe8a06 into RustPython:main Nov 20, 2021
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.

3 participants