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

Updates for new FCPath #146

Merged
merged 5 commits into from
Dec 11, 2024
Merged

Updates for new FCPath #146

merged 5 commits into from
Dec 11, 2024

Conversation

rfrenchseti
Copy link
Collaborator

@rfrenchseti rfrenchseti commented Nov 26, 2024

Major changes:

  • Change gold master tests to use new FCPath functionality from the filecache package. This returns much of the code to the way it looked before PR Changes to support rms-filecache #138 and makes the code easier to read.
  • Change all hosts to accept an FCPath as the argument to from_file and from_index. If the provided path is remote, the file is retrieved before use.
  • Change spicedb to use FCPath.

Issues:

  • Due to the lack of sufficient tests, most of the changes to the hosts modules have not been tested.

Copy link
Collaborator

@jnspitale jnspitale left a comment

Choose a reason for hiding this comment

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

All units tests pass, looks nice. Only comment I have is really about filecache: I see file retrieval is done explicitly. It seems like this could be hidden inside the class, where the local path is stored inside the class instance and retrieval would be performed if the local path is not yet defined.

Copy link
Collaborator

@markshowalter markshowalter left a comment

Choose a reason for hiding this comment

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

I will need to gain some experience with the new module, but I see no problem with your changes. Go ahead and merge.

@rfrenchseti rfrenchseti merged commit 3bb32e2 into main Dec 11, 2024
10 checks passed
@rfrenchseti rfrenchseti deleted the rf_241125_filecache branch December 11, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants