Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CloudStorage #34

Merged
merged 15 commits into from
Nov 9, 2023
Merged

CloudStorage #34

merged 15 commits into from
Nov 9, 2023

Conversation

d2hydro
Copy link
Contributor

@d2hydro d2hydro commented Nov 8, 2023

Cloud class for #32

@d2hydro d2hydro requested a review from visr November 8, 2023 17:12
@d2hydro d2hydro self-assigned this Nov 8, 2023
@d2hydro d2hydro mentioned this pull request Nov 8, 2023
Base automatically changed from codes to main November 8, 2023 17:14
@d2hydro d2hydro marked this pull request as ready for review November 8, 2023 18:32
@d2hydro d2hydro mentioned this pull request Nov 8, 2023
3 tasks
Copy link
Member

@visr visr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a commit and left some comments.


@dataclass
class Cloud:
data_dir: Union[str, Path] = RIBASIM_NL_DATA_DIR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you convert to Path in __post_init__. Can't we not drop the Union here and make sure it is Path from the start?

I prefer to be less flexible to keep this code as simple as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. From the user-perspective; most readers accept Path and str-type paths; e.g. gpd.read_file(Path("my_file.shp")) and gpd.read_file("my_file.shp") both work.
  2. It will be a string (or None) if users do or do not supply RIBASIM_NL_DATA_DIR or the data_dir at __init__
    if self.data_dir is None:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not saying we should not accept strings from the user, just that in our class it seemed like it would be easier if we convert it at the door to this class. But I don't directly have a suggestion how to do this, so feel free to ignore.

Comment on lines 47 to 48
def is_dir(item):
return Path(item).suffix == ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use Path.is_dir()? I don't think we need this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is used in

. Path("my_dir").is_dir() is only True if path is directory ánd the directory exists. If you have better code, please provide.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok it wasn't clear to me that this should be true if the directory doesn't exist. Would be good to clarify in a docstring.

src/ribasim_nl/ribasim_nl/cloud.py Outdated Show resolved Hide resolved
src/ribasim_nl/tests/test_cloud.py Outdated Show resolved Hide resolved
@visr
Copy link
Member

visr commented Nov 8, 2023

Should we perhaps use this: https://github.com/fsspec/universal_pathlib?

@d2hydro
Copy link
Contributor Author

d2hydro commented Nov 9, 2023

Should we perhaps use this: https://github.com/fsspec/universal_pathlib?

Cool, let's do it later?

Copy link
Member

@visr visr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to add the is_dir docstring, otherwise this can be merged. I made #35 for possible follow-up improvements.

- `RIBASIM_NL_DATA_DIR`: directory with your local copy of data in the Ribasim-NL cloud

## Initialize the cloud
Import the `Cloud`` and initialize it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Import the `Cloud`` and initialize it
Import the `CloudStorage` and initialize it

@visr visr changed the title Cloud CloudStorage Nov 9, 2023
@d2hydro d2hydro merged commit e148d59 into main Nov 9, 2023
3 checks passed
@d2hydro d2hydro deleted the cloud branch November 9, 2023 14:24
@d2hydro d2hydro removed their assignment Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants