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 dir() to ObjectProtocol. #886

Merged
merged 3 commits into from
May 4, 2020
Merged

Conversation

m-ou-se
Copy link
Contributor

@m-ou-se m-ou-se commented May 1, 2020

No description provided.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thank you!
Looks the error in travis is because of the version mismatch between cargo and rustc? 🤔 (weird, though)

@kngwyu
Copy link
Member

kngwyu commented May 2, 2020

@m-ou-se
Could you please rebase on the master?
#888 would resolve the travis failure.
In addition, any test for this API is appreciated.

@davidhewitt
Copy link
Member

davidhewitt commented May 2, 2020

A general comment about these kinds of functions - dir, repr etc.

At the moment they're trait methods, but I personally would prefer them to be free functions like this:

fn dir<'py, T>(obj: &T) -> &PyList
where &'py T: AsRef<PyAny>
{ ... }

// Use later as 
pyo3::dir(obj);

It's not a strong feeling, but I think it is more consistent with how these work in Python, where they're also used as dir(obj), repr(obj) etc.

Wonder how you feel about this?

(If we wanted compatibility we could leave them on ObjectProtocol but mark as deprecated?)

@kngwyu
Copy link
Member

kngwyu commented May 3, 2020

A general comment about these kinds of functions - dir, repr etc.

A bit off-topic and should be discussed in another thread?

@davidhewitt
Copy link
Member

A bit off-topic and should be discussed in another thread?

I thought it was relevant given we're adding dir now, but sure I'll move this to another issue.

@davidhewitt
Copy link
Member

Many thanks @m-ou-se for updating this PR. As I guess you have a need for dir(), I'd be interested to hear what you think of the idea proposed in #892 . (We could have pyo3::dir() and o.__dir__() in the way suggested by that PR.)

@m-ou-se
Copy link
Contributor Author

m-ou-se commented May 3, 2020

@davidhewitt Ah, didn't see that one yet. Looks good.

@davidhewitt
Copy link
Member

davidhewitt commented May 3, 2020

@davidhewitt Ah, didn't see that one yet. Looks good.

I think I pushed that PR about 20 seconds after you updated this branch! 😄 I'll wait for some more feedback to see what others think of that PR, but I will definitely make sure to add dir() .

src/objectprotocol.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Member

FWIW please do merge this PR without waiting for #892 - we might wait a little while to decide the right design there.

@kngwyu
Copy link
Member

kngwyu commented May 4, 2020

Thank you!

@kngwyu kngwyu merged commit 0f07cf8 into PyO3:master May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants