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.

Sign up for GitHub

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

Jump to bottom

daml-lf: add GenMap to archive proto #3431

Merged
merged 3 commits into from
Nov 12, 2019
Merged

daml-lf: add GenMap to archive proto #3431

merged 3 commits into from
Nov 12, 2019

Conversation

remyhaemmerle-da
Copy link
Collaborator

@remyhaemmerle-da remyhaemmerle-da commented Nov 12, 2019 •
edited
Loading

This PR advances the state of #2256.

We add GenMap type and builtins to the archive proto.

Pull Request Checklist

  • Read and understand the contribution guidelines
  • Include appropriate tests
  • Set a descriptive title and thorough description
  • Add a reference to the issue this PR will solve, if appropriate
  • Add a line to the release notes, if appropriate
  • Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

@remyhaemmerle-da remyhaemmerle-da added this to the Maintenance milestone Nov 12, 2019
hurryabit
hurryabit reviewed Nov 12, 2019
View reviewed changes
Copy link
Contributor

@hurryabit hurryabit left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The Haskell and protobuf parts look good to me. Thanks. I'll leave the final approval to the person reviewing the Scala side.

daml-lf/archive/src/main/protobuf/com/digitalasset/daml_lf_dev/daml_lf_1.proto Outdated Show resolved Hide resolved
gerolf-da
gerolf-da approved these changes Nov 12, 2019
View reviewed changes
Copy link
Contributor

@gerolf-da gerolf-da left a comment

Choose a reason for hiding this comment

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

Optimistically approving. Please check the comments.

daml-lf/validation/src/main/scala/com/digitalasset/daml/lf/validation/Typing.scala Outdated Show resolved Hide resolved
daml-lf/validation/src/main/scala/com/digitalasset/daml/lf/validation/Typing.scala Outdated Show resolved Hide resolved
@mergify mergify bot merged commit 6f5c5fd into master Nov 12, 2019
@mergify mergify bot deleted the daml-lf-genmap branch November 12, 2019 14:25
bame-da pushed a commit that referenced this pull request Nov 19, 2019
@remyhaemmerle-da @bame-da
daml-lf: add GenMap to archive proto (#3431)
a818fd3 
* add GenMap to archive proto

* Address Martin's comments

* Address Gerolf's comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cocreature cocreature cocreature left review comments

@hurryabit hurryabit hurryabit left review comments

@gerolf-da gerolf-da gerolf-da approved these changes

@leo-da leo-da Awaiting requested review from leo-da

@S11001001 S11001001 Awaiting requested review from S11001001

Assignees
No one assigned
Labels
component/daml-lf DAML-LF
Projects
None yet
Milestone
Maintenance
Development

Successfully merging this pull request may close these issues.
4 participants
@remyhaemmerle-da @cocreature @hurryabit @gerolf-da