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

Small simplifications for spin-app #1582

Merged
merged 2 commits into from
Jun 14, 2023
Merged

Small simplifications for spin-app #1582

merged 2 commits into from
Jun 14, 2023

Conversation

rylev
Copy link
Collaborator

@rylev rylev commented Jun 13, 2023

Small tweaks to spin-app after some spelunking. The following changes were made:

  • Add some docs. Even though these are internal types, it still helps to have some docs about why they're there.
  • Unrolled some type aliases. I find type aliases that are not reused generally make things harder to understand.
  • Made DynSafeDynamicHostComponent::update_data_any take just a &mut Data instead of a &mut Box<dyn Any + Send>. There doesn't seem to be any need for that more complicated type
  • Made DynamicHostComponents pub(crate)

@rylev rylev requested a review from lann June 13, 2023 12:38
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@lann
Copy link
Collaborator

lann commented Jun 13, 2023

There doesn't seem to be any need for that more complicated type

Probably not. There is a lot of "tweak it till it works then never touch it again" going on in there 😬

@lann
Copy link
Collaborator

lann commented Jun 13, 2023

Unrolled some type aliases. I find type aliases that are not reused generally make things harder to understand.

👍 I probably had these just because I was changing them so frequently trying to understand how to fit things together.

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Copy link
Collaborator

@lann lann left a comment

Choose a reason for hiding this comment

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

Thanks!

@rylev rylev merged commit 61089e9 into main Jun 14, 2023
@rylev rylev deleted the tweak-spin-app branch June 14, 2023 09:54
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