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.

Sign up for GitHub

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

Handle main_dir in staging #35

Merged
merged 1 commit into from
Apr 14, 2022
Merged

Handle main_dir in staging #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions articat/fs_artifact.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,12 @@ def main_dir(self) -> str:
"""
# to not worry about presence of scheme, we remove the prefix
# and add it back
if self.files_dir:
if self._file_prefix is not None:
return self._file_prefix
elif self.files_dir:
Comment on lines +76 to +78
Copy link
Member

Choose a reason for hiding this comment

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

So _file_prefix is short for _staging_file_prefix, e.g. it's only set for staged artifacts? And files_dir is main_dir, but only for published artifacts?

Something to think about for the future... should the whole FSArtifact be refactored such that:

  • main_dir (or files_dir... either name okay) holds the path to the staged artifact and then the published artifact
  • a bool variable like is_staging indicates whether the artifact is still unpublished
  • files_pattern is relative to main_dir and therefore doesn't need to be edited upon publication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So _file_prefix is short for _staging_file_prefix, e.g. it's only set for staged artifacts? And files_dir is main_dir, but only for published artifacts?

Yep.

Re refactor/future - def agree, there's a number of things I would like to refactor or add, but that probably won't happen anytime soon, not because I don't want to, but because it's not necessarily our priority right now. Linking to #5

return self.files_dir
# TODO: remove this code path
# TODO: remove this code path, for this to be safe to remove
# we need to update legacy artifacts first (or remove them)
# This is the legacy handling of the main_dir, where we gather
# main dir from the files_pattern
assert self.files_pattern is not None
Expand Down
1 change: 1 addition & 0 deletions articat/tests/fs_artifact_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ def test_fs_artifact_join_paths_works(uid: ID) -> None:
with fsspec.open(p, "w") as f:
f.write("ala ma kota")
a.files_pattern = None
assert a.main_dir == a.staging_file_prefix
assert p == f"{a.staging_file_prefix}/data.dat"
assert a._file_prefix is None
p = a.joinpath("data.dat")
Expand Down