Skip to content

Conversation

@guzman-raphael
Copy link
Contributor

@guzman-raphael guzman-raphael commented Nov 1, 2024

Following the merge of #34 , this PR applies the ideas in my review.

Below is a summary of all the features that were accomplished across both PRs.

Features

  • Use generic methods
  • Make deletes explicit (delete_annotation, delete_pod), remove checks in delete_pod since deletes are explicit, and prevent deletion of the last annotation in delete_annotation
  • Change annotation save path (annotation subdirectory in model directory)
  • Make annotation optional for load_pod and delete_pod but required for save_pod
  • Support loading or deleting pod from a hash or name/version (via ModelID)
  • Make Result pub
  • Expose LocalFileStore.directory only via getter
  • Use impl AsRef<Path> as opposed to impl Into<PathBuf>
  • Apply map to format desired in list_model. Currently, just passing through Vec<ModelInfo>.

Minor Improvements

  • Make path builder more general
  • Use a Vec<ModelInfo> internally when parsing annotation path
  • Compile regex once instead of in each iteration (moved out of map via move)
  • Parse class in regex
  • Make save_file more flexible
  • Use YAML content as opposed to files in from_yaml
  • Add lookup_hash based on name/version
  • Remove get_version_map since unneeded with explicit deletes
  • Remove FileHasNoParent error
  • Rename parse_annotation_path(path) -> find_annotation(glob_pattern) to improve readability.
  • Add snake_case utility
  • Simplify util methods
  • Simplify to_yaml usage
  • Update docs

Tests

Dev Improvements

  • Make spec/annotation paths and filenames easier to change in the future
  • Add spell checker w/ a word ignore list
  • Fix typos

…s since they will be implemented separately.
@guzman-raphael guzman-raphael force-pushed the improve-annotation-and-generics branch from 660f723 to dd4d681 Compare November 1, 2024 23:23
@guzman-raphael guzman-raphael marked this pull request as ready for review November 2, 2024 00:20
Copy link
Contributor

@Synicix Synicix left a comment

Choose a reason for hiding this comment

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

So there is a couple of discussion points we should probably go over next time we meet:

  • How to deal with annotationless models. Seems that we have different apporches on how to handle that
  • Test changes should adopt new design for generics?
  • What should list_model return

…generic `TestStoredModel`, add generic `basic_test` that can be reused for other models, create separate test for file existence checks, rename tests, and allow usage of expect in result in test fixtures.
…`mut` use and add `pod_load_from_hash` test.
…notation` and add better test assert messages.
…model` signature to `Vec<ModelInfo>`, and pull out implementations from functions in tests.
@Synicix Synicix merged commit a6656ea into nauticalab:dev Nov 10, 2024
1 check passed
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.

Change regex to mapping function Change Annotation handling Make Annotation optional Make Store trait methods generic

2 participants