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

Upgrade SDK to use ApplyNamespace instead of ApplyScope #183

Merged
merged 4 commits into from
May 14, 2024

Conversation

jaredoconnell
Copy link
Contributor

@jaredoconnell jaredoconnell commented May 9, 2024

Fixes #179

Changes introduced with this PR

This upgrades the SDK. This switches out a few function calls, and utilizes a convenience type for InvalidSerializationDetector.


By contributing to this repository, I agree to the contribution guidelines.

@jaredoconnell jaredoconnell changed the title Upgade SDK to use ApplyNamespace instad of ApplyScope Upgrade SDK to use ApplyNamespace instead of ApplyScope May 13, 2024
Copy link

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

I keep going back and forth between here and the SDK change. (It's annoying that, while GitHub deigns to show me reference/declaration links, it seems to be using the old code: e.g., a reference to the new addInputNamespacedScopes shows the old declaration.)

Basically, this map-of-map-of-schema.ObjectSchema is unwieldy, almost like there ought to be another level of abstraction here. Still, I don't see anything wrong...

Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

Looks generally good; however, instead of modifying anySchemaWithExpressions to keep it compatible with schema.AnySchema, please consider embedding schema.AnySchema in anySchemaWithExpressions and removing the redundant receivers. Likewise, please check the receivers of InvalidSerializationDetectorSchema and see if there are any which you can remove, now that it embeds schema.ScalarType (I think there might be several).

workflow/any.go Outdated Show resolved Hide resolved
@jaredoconnell
Copy link
Contributor Author

I agree that the map of map isn't ideal. I could try a type alias, but I don't think that's worth it. I don't think much is gained by much more abstraction.
I reduced redundancy in anyTypeWithExpressions by embedding it. All redundant functions were already removed in InvalidSerializationDetector in a prior commit.

Copy link
Contributor

@webbnh webbnh left a comment

Choose a reason for hiding this comment

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

🚢

@jaredoconnell jaredoconnell merged commit d83fbe6 into main May 14, 2024
5 checks passed
@jaredoconnell jaredoconnell deleted the use-applynamespace branch May 14, 2024 14:39
@dbutenhof dbutenhof mentioned this pull request May 17, 2024
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.

Namespaced scope fails for stress-ng plugin
3 participants