-
Notifications
You must be signed in to change notification settings - Fork 703
feat(binding/java): Add list with recursive support #5718
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
Conversation
|
i also fixed one unit test failing on my local machine, but looks like it is failing in ci. i will revert it later |
|
Considering that we will be adding more arguments for the list, how about following the same pattern to add this? #5664
fs should support |
which service should i use? do i need to verify the capability as well? i observed error when specify the i will update the code later to align with the pr you provided. |
I remember that all services implementing |
again, i may be wrong, but looks like the option for fs list is not used. so i think there is no recursive supprot for fs opendal/core/src/services/fs/backend.rs Line 364 in a6b0478
|
Hi, it's implemeneted outsides at opendal/core/src/layers/complete.rs Lines 249 to 292 in a6b0478
Sorry for making you feel confused. We have a plan to refactor this part to make it more clear. |
883f37b to
e83ea59
Compare
bindings/java/src/async_operator.rs
Outdated
|
|
||
| let path = jstring_to_string(env, &path)?; | ||
|
|
||
| let recursive = if env.call_method(&options, "isRecursive", "()Z", &[])?.z()? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i had some trouble on passing the options to the async block so that is the best way i can do. please let me know if that is not suitable, i may need some help on how to pass this arround. thanks
Xuanwo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I'm a bit confused why we need to apply CompleteLayer?
core/src/layers/mod.rs
Outdated
|
|
||
| mod complete; | ||
| pub(crate) use complete::CompleteLayer; | ||
| pub use complete::CompleteLayer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CompleteLayer is applied by default. We don't need to apply it again.
Xuanwo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @cuichenli for working on this. This PR looks good to me. Wait for @tisonkun and @G-XD for another look.
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
|
Make some code style change and approved. Keep all the binding mappings follow a similar structure and avoid some unnecessary clone. |
|
Thank you @cuichenli and @tisonkun for working on this! |
Rationale for this change
Currently, the java binding only support list current directory, it would be nice to have the ability to list recursively. Given opendal itself already supported this, I think it is reasonable to add this to the java binding as well. But I am not sure if that is suitable, so please feel free to correct me if I missed anything. It is a little bit tricky to add the unit test directly, given
fsservice it self does not support recursively list. But I have tested in my java program with s3 and gcs, they are working as expected.What changes are included in this PR?
Added one paramater to support list recursively. Also fixed one unit test.
Are there any user-facing changes?
Java binding's list API changed.