[GitHub] [arrow] jorisvandenbossche commented on a diff in pull request #12848: ARROW-16159: [C++][Python] Allow FileSystem::DeleteDirContents to succeed if the directory is missing

[GitBox <gi...@apache.org>]

jorisvandenbossche commented on code in PR #12848:
URL: https://github.com/apache/arrow/pull/12848#discussion_r851327053


##########
python/pyarrow/fs.py:
##########
@@ -346,18 +346,24 @@ def create_dir(self, path, recursive):
     def delete_dir(self, path):
         self.fs.rm(path, recursive=True)
 
-    def _delete_dir_contents(self, path):
-        for subpath in self.fs.listdir(path, detail=False):
+    def _delete_dir_contents(self, path, missing_dir_ok):
+        try:
+            subpaths = self.fs.listdir(path, detail=False)
+        except FileNotFoundError:
+            if missing_dir_ok:
+                return
+            raise
+        for subpath in subpaths:
             if self.fs.isdir(subpath):
                 self.fs.rm(subpath, recursive=True)
             elif self.fs.isfile(subpath):
                 self.fs.rm(subpath)
 
-    def delete_dir_contents(self, path):
+    def delete_dir_contents(self, path, missing_dir_ok):

Review Comment:
   I suppose this will break existing implementations, because our C++ code will now pass this keyword, while existing handlers won't yet accept that keyword in this method like above. 
   
   Now, that will _always_ happen if we add new keywords, and we want to be able to add new keywords (so not saying we shouldn't do this change). But therefore wondering:
   
   - Should we recommend adding `**kwargs` (that get ignored) to all methods on the Handler class to avoid such breakage? (of course, that would ignore the keyword initially when we add one, so also result in unexpected behaviour for the user. So that is maybe not better as initially raising an error until the handler gets updated for the latest arrow version)
   - In theory we could only pass the keyword in the C++ code (in `PyFileSystem::DeleteDirContents`) to the handler method _if_ it has a non-default value, and otherwise not pass it (and set a default on the line above). Of course that complicates the C++ code with an if/else block (and introduces a risk of introducing inconsistent defaults at some point if one would change).
   



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org