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 let rebind bug. #1647

Merged
merged 3 commits into from Jul 9, 2018

Conversation

Projects
None yet
3 participants
@gilch
Member

gilch commented Jun 28, 2018

fixes #1645

Also adds a test for the issue, and cleans up the two files.--proper special indent for let tests, and removes some outdated comments in the walk module.

@Kodiologist

This comment has been minimized.

Member

Kodiologist commented Jun 28, 2018

Could you spin out the indentation changes into their own commit? That would make it easier to see the substantive changes.

@gilch

This comment has been minimized.

Member

gilch commented Jun 28, 2018

The only new test is at the bottom test-let-rebind everything else in that file should just be whitespace.

I also removed the big comment at the bottom of walk. The rest is more substantive.

@Kodiologist

Okay, then, looks good.

@Kodiologist

This comment has been minimized.

Member

Kodiologist commented Jun 28, 2018

While it occurs to me to ask, does let and its subroutines use destructive modification of models, and hence need to be updated if we do #1542?

@gilch

This comment has been minimized.

Member

gilch commented Jun 28, 2018

Rebased so the final commit shows substantive changes.

@gilch

This comment has been minimized.

Member

gilch commented Jun 28, 2018

Oh it was already approved.

does let and its subroutines use destructive modification of models

I do not recall in enough detail. It was based on the earlier stuff in the walk module, which was pretty functional-looking. I think it mostly rebuilt everything as it walked over it, which should work fine with immutable models. Unless some of that building mutated the new things. It shouldn't be too hard to adapt anyway.

@Kodiologist Kodiologist referenced this pull request Jul 5, 2018

Merged

Hy 0.15.0 #1652

@kirbyfan64

This comment has been minimized.

Contributor

kirbyfan64 commented Jul 8, 2018

@gilch Ready for merge...based on your comment, it seems like there's nothing else to really do? (Just checking first.)

@gilch

This comment has been minimized.

Member

gilch commented Jul 8, 2018

Yeah, it's just a bugfix, so I don't think we need any change in the docs. I'm not sure if it's worth adding to NEWS, but that tends to create merge confilcts.

@gilch gilch merged commit 0de8557 into hylang:master Jul 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gilch

This comment has been minimized.

Member

gilch commented Jul 9, 2018

I'm just going to merge it. NEWS can go in #1652, if necessary.

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