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

Clean up imports; fixup a couple missed errors #3

Merged
merged 1 commit into from
Feb 8, 2024
Merged

Conversation

amisevsk
Copy link
Contributor

@amisevsk amisevsk commented Feb 8, 2024

  • Add aliases to oci imports (i.e. use 'ocispec' instead of 'v1')
    • This matches what e.g. oras does internally and makes it easier to track where definitions are coming from (e.g. our store vs. their store)
  • Replace functions that create empty structs (same thing as just creating the struct directly)
    • Maybe me being opinionated here, but in Go the struct is public and we don't need new() functions unless there's initialization that needs to happen.
  • Catch a missed error in build that hid the fact that running build twice fails

* Add aliases to oci imports (i.e. use 'ocispec' instead of 'v1')
* Replace functions that create empty structs (same thing as just
  creating the struct directly)
* Catch a missed error in build
@amisevsk
Copy link
Contributor Author

amisevsk commented Feb 8, 2024

Catch a missed error in build that hid the fact that running build twice fails

❯ jmm build -f examples/onnx/Jozufile .
build called
sha256:f23d3cc937341a84aedcd7a0f06654aa656676c3b2222d5435aa361b5c183a62: application/vnd.jozu.model.config.v1+json: already exists

Copy link
Contributor

@gorkem gorkem left a comment

Choose a reason for hiding this comment

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

lgtm. Good call on the New functions, I started those thinking I would need to set some protected items but we cross that bridge when we get there

@gorkem gorkem merged commit ea74211 into main Feb 8, 2024
1 check passed
@gorkem gorkem deleted the cleanup-imports branch February 8, 2024 15:37
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.

2 participants