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

(reset-thing ...) doesn't work with new typed things #58

Closed
jarcane opened this issue Aug 2, 2019 · 4 comments
Closed

(reset-thing ...) doesn't work with new typed things #58

jarcane opened this issue Aug 2, 2019 · 4 comments

Comments

@jarcane
Copy link
Owner

jarcane commented Aug 2, 2019

The implementation of reset-thing is incorrect. It works by extending the old thing and trying to rewrite the fields that way.

But this doesn't work anymore with the new (soon to be released) typed thing implementation. I'm not sure if this counts as a bug in things themselves, or in just the weird way reset-thing gets around the lack of a by-name field assignment syntax (see #46) by using thing extends.

Either way warrants investigation.

@jarcane jarcane added the bug label Aug 2, 2019
@jarcane
Copy link
Owner Author

jarcane commented Aug 5, 2019

For reference, the error is trivially reproducible:

> (describe Foo (bar 1))
> (describe Baz extends Foo (bar 1))
. . >: contract violation
  expected: real?
  given: #<procedure:this>
  argument position: 1st
  other arguments...:

It happens any time you attempt to create a child of a thing with a field of the same name as one of the parent thing's fields.

@jarcane
Copy link
Owner Author

jarcane commented Aug 5, 2019

On further investigation, this is actually the result of much deeper problems than I realize, which is that a parent thing's types are not properly passed down to the child, so everything goes all squibbly. Time to deal with that can of worms I guess.

jarcane added a commit that referenced this issue Aug 5, 2019
This fixes #58 by correcting the implementation of alist-merge to prevent the error.

The larger question of whether it should remains unanswered.
@jarcane
Copy link
Owner Author

jarcane commented Aug 5, 2019

This is fixed. I'm not sure philosophically if it should be, but ... it's fixed, for the sake of not breaking backwards compatibility. But I think the by-name copy syntax in #46 is a better solution, and should be pursued as a better implementation (and maybe some error-checking added to things to prevent this case)

@jarcane jarcane closed this as completed Aug 5, 2019
@jarcane jarcane mentioned this issue Aug 5, 2019
1 task
jarcane added a commit that referenced this issue Aug 9, 2019
@jarcane
Copy link
Owner Author

jarcane commented Aug 9, 2019

This is now fixed further in #57 by no longer relying on inheritance to update the thing and instead use the copy syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant