Xuanwo commented on code in PR #7114:
URL: https://github.com/apache/opendal/pull/7114#discussion_r2683393774


##########
core/services/onedrive/src/lister.rs:
##########
@@ -29,6 +29,32 @@ use super::graph_model::GENERAL_SELECT_PARAM;
 use super::graph_model::GraphApiOneDriveListResponse;
 use super::graph_model::ItemType;
 
+// --- Helper Function for URL Generation (Logic Extracted Here) ---
+// We use a closure `item_url_fn` to mock the behavior of `onedrive_item_url`
+// so we can test this without a real OneDriveCore instance.
+fn build_request_url(

Review Comment:
   Please don't extract helper functions in this way. If we do want to assert 
if the generated URL correctly. 
   
   We can split APIs in to `xxx_request() -> http::Request` and `xxx()` which 
will call `xxx_request()` inside.
   
   In this way, we just need to call `xxx_request()` and check the generated 
requests.



##########
core/services/onedrive/src/lister.rs:
##########
@@ -130,10 +140,6 @@ impl oio::PageList for OneDriveLister {
             let last_modified = 
drive_item.last_modified_date_time.parse::<Timestamp>()?;
             meta.set_last_modified(last_modified);
 
-            // When listing a directory with `$expand=versions`, OneDrive 
returns 400 "Operation not supported".

Review Comment:
   The same.



##########
core/services/onedrive/src/lister.rs:
##########
@@ -83,18 +99,13 @@ impl oio::PageList for OneDriveLister {
 
         // Include the current directory itself when handling the first page 
of the listing.
         if ctx.token.is_empty() && !ctx.done {
-            // TODO: when listing a directory directly, we could reuse the 
stat result,

Review Comment:
   Please don't touch unrelated code.



##########
core/tests/behavior/async_list.rs:
##########
@@ -34,6 +34,7 @@ pub fn tests(op: &Operator, tests: &mut Vec<Trial>) {
             op,
             test_check,
             test_list_dir,
+            test_list_root_dir,

Review Comment:
   We do not need this test, and it does not work in practice because all our 
behavior tests set a random root directory during tests.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to