Set rm_files to be a synchronous method #503
Conversation
| @@ -1248,7 +1248,7 @@ async def _rm_files( | |||
| for file in file_paths: | |||
| self.invalidate_cache(self._parent(file)) | |||
|
|
|||
| sync_wrapper(_rm_files) | |||
| rm_files = sync_wrapper(_rm_files) | |||
There was a problem hiding this comment.
Ah this is something that I missed as well when we went over the original GitHub feature request... Based on the fsspec specification there is no rm_files() shared interface; there's only an rm_file(). The original GitHub issue: #497 is also only requesting for rm_file().
So, it is not appropriate to be setting a sync wrapper like this because it does not appear rm_files to be a shared interface across file systems. Instead, it would probably make sense to add an async _rm_file that is a simplified wrapper over the _rm implementation to implement the feature request.
Even more interesting, it seems there used to be a _rm_file() implementation prior to this PR: #383 and because the underlying AsyncFileSystem mirrors methods, I suspect that adlfs might have actually at one point supported rm_file() and could be a regression. It would be great to confirm if adlfs ever supported rm_file() in a version prior to that PR.
This addresses the github issue where rm_files is not implemented. This was a simple fix where sync_wrapper(_rm_files) just needed to be set to rm_files. Tests were also added to make sure it works as expected.