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

Add set binding #3

Merged
merged 2 commits into from
May 21, 2017
Merged

Add set binding #3

merged 2 commits into from
May 21, 2017

Conversation

messense
Copy link
Member

@messense messense commented May 18, 2017

TODO:

  • frozenset?
  • add more tests

Ref #2

Currently this project is a little hard to get involved because of lacking development guide docs.

@messense
Copy link
Member Author

I am not sure how to implement:

  • set iterator
  • set union, issuperset, etc. methods.

@fafhrd91
Copy link
Contributor

I think PyIterator instance is fine

@fafhrd91
Copy link
Contributor

lgtm on implementation

do you want to implement iter()?

@messense
Copy link
Member Author

Switched Travis CI distribution to trusty.

https://blog.travis-ci.com/2017-04-17-precise-EOL

/// Creates a new set.
///
/// May panic when running out of memory.
pub fn new(py: Python, elements: &[PyObject]) -> PySet {
Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think if we change this to:

pub fn new<T: ToPythonObject>(py: Python: elements: &[T]) -> PySet {}

so that we can do:

let set = PySet::new(py, &[1, 2, 3]);

instead of

let set = PySet::new(py, &[1.to_py_object(py).into_object()]);

Copy link
Contributor

Choose a reason for hiding this comment

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

I think, this is very good idea

Copy link
Member Author

Choose a reason for hiding this comment

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

But other things like PyList still uses pub fn new(py: Python, elements: &[PyObject]) -> PyList, I guess we should do this in a separate PR to do them all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should have write access to repo. this kind of change you can just commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll merge this then do other things.

@messense messense merged commit 3401683 into PyO3:master May 21, 2017
@messense messense deleted the feature/set-binding branch May 21, 2017 05:25
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