aai-institute / lakefs-spec

An fsspec implementation for the lakeFS project.
http://lakefs-spec.org/
Apache License 2.0
37 stars 4 forks source link

Replace `fs._open()` with proper `fs.open()` implementation #250

Closed nicholasjng closed 5 months ago

nicholasjng commented 5 months ago

This is sensible because a) the builtin fs.open() was giving wrong type hints of AbstractBufferedFiles, which we no longer use, and b) the new lakeFS APIs allow native handling of non-bytemode open(), which the base fs.open() needs a TextIOWrapper for.

No new tests, since all file-system APIs are already being tested. What is missing are integration tests to pandas, polars, etc., since those all rely on fs.open().

nicholasjng commented 5 months ago

For reviewers:

1) Should we explicitly keep allowing {r,w,x}t modes if the only thing we do is silently coerce them into {r,w,x}b? We have one such example in the readme code, which would need to be updated. 2) What's your feeling on finally adding an integration test for the data science zoo (see the commit message)?

EDIT: Seems that if we allow text mode, we should instead coerce to r,w,x instead of their bytemode counterparts. I'll update the PR.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (4ebee87) 94.07% compared to head (8d1076d) 94.10%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #250 +/- ## ========================================== + Coverage 94.07% 94.10% +0.02% ========================================== Files 5 5 Lines 405 407 +2 Branches 70 72 +2 ========================================== + Hits 381 383 +2 Misses 15 15 Partials 9 9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.