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

feat!: migrate schema to core #273

Merged
merged 10 commits into from
Apr 7, 2023
Merged

feat!: migrate schema to core #273

merged 10 commits into from
Apr 7, 2023

Conversation

Gozala
Copy link
Collaborator

@Gozala Gozala commented Apr 7, 2023

Overview

  • Migrates schema from validator to the core
    • I needed it in the transport layer to deal with workflow versions etc.. and pulling in validator there made no sense.
    • Creating separate PR only with this to reduce amount of changes bundled with transport logic
  • Adds Variant schema for working with Variant types

@Gozala Gozala requested a review from gobengo April 7, 2023 00:27
@Gozala Gozala requested a review from travis April 7, 2023 00:34
* Schema for the {@link Variant} types, which is a special kind of union type
* where every choice is a struct with a single field denoting the choice.
*
* The `_` branch is a special case which can be used to represent all other
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we have to have the _ special case, instead of specifying this default behavior as a whole other parameter not inside VariantChoices?
One downside is it seems to preclude using _ as a variant name (not a huge loss, but seems worth avoiding if possible). Is there an upside to specifying the default behavior via a reserved key name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is very common in typed languages with pattern matching to use _ as a "default" branch e.g. rust does this, elm does this haskell does this, even ucan invocation spec does it in some sense, so it seemed reasonable to do it here.

One downside is it seems to preclude using _ as a variant name (not a huge loss, but seems worth avoiding if possible).

I think using _ as branch name is bad enough idea that I don't actually feel it's a real loss.

specifying this default behavior as a whole other parameter not inside VariantChoices

It is possible and I have tried that in fact, but it made inference types and variant function a lot more complicated and brittle because you ended up with this unnamed branch. In the end it felt clunky so I've chose to go with _ instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

but it made inference types and variant function a lot more complicated and brittle

I was guessing maybe this was why (but wanted to confirm), and +1 for avoiding the brittleness.
I didn't realize that _ is common in other languages. Thanks for explaining that.

@Gozala Gozala merged commit ce95504 into main Apr 7, 2023
@github-actions github-actions bot mentioned this pull request Apr 7, 2023
@heyjay44 heyjay44 mentioned this pull request Apr 11, 2023
23 tasks
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