[pkdash]

TODO:

• Add test for each of the supported python data objects
• Update API documentation
• Add example notebook

[porterhyphenborden]

This is awesome and you took it further than just zarr/xarray! I think though that this code could be refactored a bit to use a pattern that is easier to extend and less brittle than the multiple conditionals in this method.

We could do something like a base reader class with child classes for each reader type. The reader classes each document what extensions they support, and implement a read() method. Something like:

class StreamReader:
    extensions = ()
    default_object_type = None
    object_types = ()

    def validate_object_type(self, object_type: str | None) -> str:
        ...
        (returns object type, default or selected if valid)

    def read(self, hs, s3_path: str, reader_kwargs: dict[str, Any]) -> Any:
        raise NotImplementedError

and then a child class implements the read method:

class CsvReader(StreamReader):
    extensions = (".csv",)
    default_object_type = "pandas"
    object_types = ("pandas",)

    def read(self, hs, s3_path, reader_kwargs):
        pd = hs._require_module(pandas, "pandas")
        with hs._s3_client.open(s3_path) as f:
            return pd.read_csv(f, **reader_kwargs)

It becomes very obvious how to add a new reader and the code is a bit more self-documenting. And we don't have to worry about breaking the conditional logic. We can register each reader to an extension:

STREAM_READERS = {
    ".csv": CsvReader(),
    ".parquet": ParquetReader(),
  ...
}

Fetch the reader for the extension, and then call it's methods to validate the object type and read in the data. Logic for each reader stays local to its class.

Apologies for being so prescriptive -- I don't think it has to be implemented exactly like this, but I wanted to give a concrete example so my suggestion wasn't too confusing!

What do you think?

[pkdash]

Yeah it is much better. Thanks!

[porterhyphenborden]

Thank you for refactoring -- lgtm!

[porterhyphenborden]

This is totally fine for now, just leaving a note for future expansion -- we could likely handle .txt, .psv, and .tsv here as well but we would need a way to accept a separator argument!