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

Fix NewValue checks to be properly recursive #79

Merged
merged 5 commits into from
Apr 21, 2021
Merged

Conversation

paddycarver
Copy link
Contributor

Fix the checks that NewValue runs to ensure that a value can be used
with a Type to properly handle DynamicPseudoTypes nested in complex
types. Builds on and supersedes #76.

Should we make this part of the behavior for .Is? My gut says "no",
because Is is supposed to be about checking whether a type is
semantically similar, not that it can be used as another type. Maybe we
want a Fulfills method, similar to Is, that does this? I don't know
that this behavior needs to be exported, though...

Fixes a bug in #74 that would panic for complex types containing
DynamicPseudoTypes.

@paddycarver paddycarver added the bug Something isn't working label Apr 18, 2021
@paddycarver paddycarver requested a review from a team April 18, 2021 11:39
paultyng and others added 5 commits April 20, 2021 14:46
Fix the checks that NewValue runs to ensure that a value can be used
with a Type to properly handle DynamicPseudoTypes nested in complex
types. Builds on and supersedes #76.

Should we make this part of the behavior for `.Is`? My gut says "no",
because `Is` is supposed to be about checking whether a type is
semantically similar, not that it can be used as another type. Maybe we
want a `Fulfills` method, similar to Is, that does this? I don't know
that this behavior needs to be exported, though...

Fixes a bug in #74 that would panic for complex types containing
DynamicPseudoTypes.
Add a test to exercise the object-with-a-dynamic-attribute logic we
fixed.
Add tests for all the complex types, and make sure types that require
elements be the same type get only elements of the same exact type.
Copy link
Member

@kmoe kmoe 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. We can always export UseTypeAs/Fulfils or change Is behaviour later.

@paddycarver
Copy link
Contributor Author

I would like to, in 0.4.0, do some work on Is to make this cleaner and have less magic behavior:

  • Is is a shallow check, that root types are the same type
  • Equals is a deep check, that root types and all elements and attributes are the same type
  • ConformsTo is a deep check, that root types and all elements and attributes conform (are equivalent, or a concrete type is being substituted for DynamicPseudoType)

I just don't want to hold up 0.3.0 for that work, it's a big enough release already. I'll write up an issue for it.

@paddycarver paddycarver merged commit 012c7f0 into main Apr 21, 2021
@paddycarver paddycarver deleted the paddy_fix_newvalue branch April 21, 2021 09:23
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants