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

cleanup: manifest improvements #42

Merged
merged 2 commits into from
Nov 29, 2023
Merged

cleanup: manifest improvements #42

merged 2 commits into from
Nov 29, 2023

Conversation

zshipko
Copy link
Contributor

@zshipko zshipko commented Nov 29, 2023

@@ -217,6 +228,52 @@ type Manifest struct {
Timeout uint64 `json:"timeout_ms,omitempty"`
}

type concreteManifest struct {
Copy link
Member

@nilslice nilslice Nov 29, 2023

Choose a reason for hiding this comment

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

I haven't tested this, but we could potentially embed the Manifest struct into this one (with no field name) and add the Wasm []concreteWasm field to it.

This way they stay in sync and we avoid duplication. Totally may not work though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did try that but got a weird stack overflow error

Copy link
Member

Choose a reason for hiding this comment

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

haha hmm ok, we can always change it later if we want since it's all internal. not worth spending much time now just to avoid a tiny bit of tech debt 😅

Copy link
Member

@nilslice nilslice left a comment

Choose a reason for hiding this comment

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

LGTM! No issue merging now but maybe @mhmd-azeez can take a peek at some point too that would be great.

}

func (m *Manifest) UnmarshalJSON(data []byte) error {
tmp := concreteManifest{}
Copy link
Member

Choose a reason for hiding this comment

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

oh I think this is where the stack overflow happens since UnmarshalJSON gets called recursively when it encounters the embedded type

Copy link
Member

Choose a reason for hiding this comment

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

(but I don't think there's a way around it?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, other than splitting all the Manifest fields out into another struct (which ruins the usability from Go) there doesn't seem to be a good way to re-use them. I will sleep on it though.

@zshipko
Copy link
Contributor Author

zshipko commented Nov 29, 2023

Thanks! I will leave it open and merge it tomorrow so he gets a chance to review.

Copy link
Collaborator

@mhmd-azeez mhmd-azeez left a comment

Choose a reason for hiding this comment

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

Looks good, I added a basic unit test to make sure everything works and to act as an example for people in the future.

@zshipko zshipko merged commit 055ce19 into main Nov 29, 2023
3 checks passed
@zshipko zshipko deleted the cleanup-manifest branch November 29, 2023 17:13
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