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 more file-like methods to the python bindings #4384

Merged

Conversation

mobiusklein
Copy link
Contributor

This adds the following methods to the File type in the Python bindings:

  • flush
  • readable
  • writable
  • seekable
  • closed
  • readinto

For AsyncFile, the following were added:

  • readable
  • writable
  • seekable
  • closed

Manually testing was done locally, but I couldn't get the unit tests to run due to environment dependency issues. Hopefully CI will trigger here.

@github-actions github-actions bot requested review from G-XD and morristai March 22, 2024 02:11
@mobiusklein
Copy link
Contributor Author

This partially addresses #4384. It assumes that all readable services support seeking, which may not be valid, but it mirrors the implementation seek.

@messense
Copy link
Member

It assumes that all readable services support seeking

Can you use https://docs.rs/opendal/latest/opendal/struct.Capability.html#structfield.read_can_seek?

@Xuanwo
Copy link
Member

Xuanwo commented Mar 22, 2024

This is an ongoing RFC related to this: #4382

@mobiusklein
Copy link
Contributor Author

I've added the capability check to the Python bindings, but is there anything specific I should do w.r.t. the RFC? Or is it just a blocker for this PR?

@Xuanwo
Copy link
Member

Xuanwo commented Mar 22, 2024

Or is it just a blocker for this PR?

After this RFC been implemented (which is already going on), we might need to change the way of implement File in python.

@Xuanwo
Copy link
Member

Xuanwo commented Apr 10, 2024

Hi, @mobiusklein, sorry for the long waiting. I feel like this PR is good to go. Would you like to fix the CI issues so we can get it merged?

@mobiusklein
Copy link
Contributor Author

Is it correct to say that seek operations are now supported universally since the behavior is no longer described by the Capability type? I don't see any documentation about it except for an outdated note in https://opendal.apache.org/docs/rust/opendal/struct.Capability.html#naming-style.

@Xuanwo
Copy link
Member

Xuanwo commented Apr 13, 2024

Is it correct to say that seek operations are now supported universally since the behavior is no longer described by the Capability type?

Yes!

@mobiusklein
Copy link
Contributor Author

Thank you. I've refactored the bindings to reflect that. The Capability struct the File types carry around now probably is no longer relevant but I left it there for the future. I'll have to re-digest what the capabilities mean but on my first pass I don't see a way they'd intrude into the Python API.

@mobiusklein
Copy link
Contributor Author

Things appear to be all set now. Thank you.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Xuanwo Xuanwo merged commit 962d810 into apache:main Apr 13, 2024
53 checks passed
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