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

constructors === id #256

Merged
merged 6 commits into from Nov 21, 2018
Merged

constructors === id #256

merged 6 commits into from Nov 21, 2018

Conversation

FintanH
Copy link
Collaborator

@FintanH FintanH commented Oct 26, 2018

Phase 2 of #244

Changes constructors to the identity function. Marking it as deprecated and a the projection operator . should be used instead.

standard/semantics.md Outdated Show resolved Hide resolved
standard/semantics.md Outdated Show resolved Hide resolved
@f-f
Copy link
Member

f-f commented Oct 26, 2018

@Gabriel439 since this is technically breaking should we have a (minor) release with #249 first (that is, before merging this) so there's a migration path?

@Gabriella439
Copy link
Contributor

@f-f: Alright, then let's defer merging this until after the next release to ease the migration process

@FintanH
Copy link
Collaborator Author

FintanH commented Nov 14, 2018

@f-f This is good to revisit right? I almost forgot about it until I was telling a friend about Dhall and he saw I had a branch 😅

@f-f
Copy link
Member

f-f commented Nov 14, 2018

@FintanH right, it's time for a new release, which I'm not sure if it's going to be breaking (e.g. I think 55475f7 is breaking?)
So if the release is going to be breaking we can merge before releasing, otherwise we should release a minor version and then merge this.

In the meanwhile tests got moved here, so I'd ask you to add some tests to cover this change 😄

@FintanH
Copy link
Collaborator Author

FintanH commented Nov 14, 2018

@f-f Added the identity test under normalization 😸

@FintanH
Copy link
Collaborator Author

FintanH commented Nov 15, 2018

Discussed with @f-f and our plan is to prepare for a release on 19th of Nov since this contains breaking changes. Does this line up with you @Gabriel439?

@f-f
Copy link
Member

f-f commented Nov 15, 2018

I'll clarify that if the upcoming release is not breaking (i.e. minor) we should release first and then merge, otherwise we'll merge this in the upcoming release.
Another possibility is that we tag the minor release to af53b07, but this would probably mean there won't be a paired dhall-haskell release

@Gabriella439
Copy link
Contributor

@f-f: #263 is a breaking change that has been introduced since the last release

However, I still think we should cut a release before merging this, regardless of other breaking changes, so that people have a smooth migration path for constructors-related code

@Gabriella439
Copy link
Contributor

@FintanH: Yeah, feel free to cut a release. I'm prepared to cut a release of the Haskell implementation at any time. I usually cut releases for both the standard and the Haskell implementation roughly every 30 days, and it's been almost 30 days since the last release.

@FintanH
Copy link
Collaborator Author

FintanH commented Nov 16, 2018

Ok, cool. Just so I'm clear on this, I need to:

  • update this branch ✔️
  • bump the version to 4.0.0
  • wait 3 days and merge?

@Gabriella439
Copy link
Contributor

Gabriella439 commented Nov 16, 2018

@FintanH: What I mean is that we should first cut a 4.0.0 release containing just the change that adds support for UnionType.foo from #249 (i.e. the 4.0.0 release would not include the change in this branch). Then after that release we would merge in this branch which would be made available as part of release 5.0.0 (whenever we cut that, possibly in another 30 days).

In the intervening time between cutting the 4.0.0 release and the 5.0.0 release, though, this behavior could still be available for you to use on master for the Haskell implementation.

@FintanH
Copy link
Collaborator Author

FintanH commented Nov 16, 2018

Ahhh cool, makes sense! We're not in a rush to get it since we figured out the merging of records issue :)

@FintanH FintanH merged commit 7d8bdb9 into master Nov 21, 2018
@FintanH FintanH deleted the fintan/constructors-id branch November 21, 2018 11:19
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.

None yet

3 participants