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

More NonEmpty functions #10930

Merged
merged 4 commits into from
Sep 30, 2021
Merged

More NonEmpty functions #10930

merged 4 commits into from
Sep 30, 2021

Conversation

leonidr-da
Copy link
Contributor

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
  • Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
  • 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.

@leonidr-da
Add NonEmpty cons.
550e42c
@leonidr-da
Add NonEmpty find.
e72ff5d
@leonidr-da
Add NonEmpty delete, deleteBy.
92f7b02 
CHANGELOG_BEGIN
Add cons, find, delete and deleteBy functions to NonEmpty module.
CHANGELOG_END
@leonidr-da
Copy link
Contributor Author

I was also going to add a fromList to a DA.NonEmpty.Total but per https://discuss.daml.com/t/to-abort-or-to-fail/3012/8, I'm not certain whether we should ActionFail or CanAbort.

cocreature
cocreature reviewed Sep 20, 2021
View reviewed changes
compiler/damlc/daml-stdlib-src/DA/NonEmpty.daml Outdated

-- | The 'deleteBy' function behaves like 'delete', but takes a
-- user-supplied equality predicate.
deleteBy : (a -> a -> Bool) -> a -> NonEmpty a -> Optional (NonEmpty a)
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered returning [] instead? Optional (NonEmpty a) doesn’t provide any extra info or type safety.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I had not considered it. I think given the trade-off of unwrapping an Optional or recreating the NonEmpty I prefer the former. Handling the case where you delete the last element is rarer than deleting an element or deleting none. But, yes, that type does look a bit weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cocreature Is this ok with you? It would be nice to get this done.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry missed that, I think I have a preference for the [a] case. The type signature here just looks confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you say why it looks confusing? After this delete one will have to handle either a None or a [], and there are plenty of examples where we wrap in Optional to deal with the "missing" case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, I opt for the simplest type that expresses the properties I care about. In this case [a] is objectively simpler than Optional (NonEmpty a) (both at the type and at the value level) but the more complex type provides zero extra information.

While Haskell doesn’t have delete on NonEmpty it has filter which is close enough and follows my suggestion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that simplicity is the only thing one should consider. I do think that Optional (NonEmpty a) is more explicit in expressing to the user that some operation may not return a NonEmpty a. But I won't belabor this.

cocreature
cocreature approved these changes Sep 29, 2021
View reviewed changes
Copy link
Contributor

@cocreature cocreature left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

@cocreature cocreature enabled auto-merge (squash) September 30, 2021 07:14
@cocreature cocreature merged commit 3cbd922 into main Sep 30, 2021
@cocreature cocreature deleted the more_ne_functions branch September 30, 2021 09:13
azure-pipelines bot pushed a commit that referenced this pull request Oct 6, 2021
release 1.18.0-snapshot.20211006.8003.0.cfcdc13c
91a03ec 
This PR has been created by a script, which is not very smart
and does not have all the context. Please do double-check that
the version prefix is correct before merging.

@adriaanm-da is in charge of this release.

Commit log:
```
cfcdc13 Add deprecation output for sandbox classic (#11119)
ac5c52d DPP-609 Add StorageBackendTestsDeduplication (#11088)
55af7ad HttpServiceTestFixture provides a sandbox-classic ledger used for performance tests (#11128)
63ab3f3 Fix change ID references in services.rst (#11130)
31db15d Port error code changes (#11113)
9fd8182 move reusable functions from ContractsFetch to new fetch-contracts library (#11057)
c2084f6 Try and improve the explanation of compatibility test exclusions (#11114)
4df9b7c Rename command completion dedup time to duration in participant code [KVL-1057] (#10901)
11bc22d LF: builtins to create, signatory, and obersver on interface payload. (#11120)
d4cb1f9 KV ledgers: disable participant side command de-duplication [KVL-1083] (#11095)
b9a5a83 Minor: fix trace logging in `TimeBasedWriteSetSelector` (#11117)
018e908 [JSON-API] Make key_hash idx non unique  (#11102)
429f437 Remove HA indexer integration test on H2 (#11104)
9cffa1f LF: check LF transaction protobuf aginst local snapshots (#11064)
d7ee278 Optional table prefix for trigger service  (#11047)
268b2d3 Command deduplication - test case that uses CommandService and CommandSubmissionService [KVL-1106] (#11098)
fac05f6 bypass value enrichment for the performance test (#11112)
4860271 Port file was not written to... where? (#11108)
a8d703b Increase akka server request-timeout for HttpServiceIntegrationTests (#11109)
37d94aa Add unit for daily performance tests (#11107)
eb4ac8a Increase timeouts for resetting database (#11103)
db75f7d interface methods: Scala Typechecker (#11097)
d3e6f16 [JSON-API] Migrating tests to use sandbox next (#11034)
ffc8d68 Data dependencies deserialize imports (#11037)
4b31bf0 update compat versions for 1.17.0 (#11075)
9fd6326 interfaces: consuming/non-consuming iface choices (#11009)
229ce47 Data dependencies serialize imports (#11038)
5d9ec65 Add test case for parallel command deduplication using mixed clients [KVL-1090] (#11093)
d64d965 kvutils: Print state updates before and after normalization. (#11096)
be216aa update NOTICES file (#11089)
517e866 Extract common code for command dedup conformance tests [KVL-1090] (#11092)
7a4963b add `--max-inbound-message-size` flag to `daml ledger export` (#11087)
26d10b2 Drop NodeId type parameter (#10921)
3cbd922 More NonEmpty functions (#10930)
bcd4686 interface methods: Speedy (#11076)
85adaab [DPP-417][DPP-595] Error codes switching - follow-up (#11074)
d7b280b Remove debug print in daml-lf/interpreter build (#11091)
1ed6428 LF: move archive snapshots in a separate directory (#11081)
5e424f8 Command dedup conformance suites readme (#11051)
8290347 DPP-496 HA indexer integration test (#11033)
735c309 Switch from InputStream to Byte Array at the binary content JDBC transport (#11072)
6ae3afa fix perf reporting (#11073)
bf801a6 Fix SimpleDeduplicationBasic when participant deduplication is disabled (#11083)
62234dc [Short] Remove unused code (#11079)
c1d1521 interface methods: Scala AST (#11070)
7dd9c2d Remove expectations for internal failures from parallel command dedup tests (#11061)
5112599 Explicit discard in daml-lf/interpreter (#11067)
554b36c rotate release duty after 1.18.0-snapshot.20210928.7948.0.b4d00317 (#11059)
d156964 Release SDK 1.17.0 (#11062)
6d8cf70 Handle interface methods in LF conversion (#11054)
87ecf1f daml-ledger: better errors for non-json responses. (#11055)
ec2d26f [Divulgence pruning] Fixes divulgence pruning offset update query (#11046)
f13c6d6 release 1.18.0-snapshot.20210928.7948.0.b4d00317 (#11058)
d1805a3 More dependabot fun (#11063)
07b273a update NOTICES file (#11060)
```
Changelog:
```

- [JSON-API] make key_hash indexes non-unique, this fixes a bug where a duplicate key conflict was raised on the query store when the same contract was being witnessed twice by two separate parties

* [Trigger Service] Enable the new `tablePrefix` setting in the `--jdbc`
  flag to add a prefix to all tables used by the trigger service to
  avoid collisions with other components using the same db-schema.
* [Trigger Service] Enable the new ``--allow-existing-schema`` flag to
  initialize the trigger service on a database with a pre-existing
  schema.

- [Daml Assistant] Add support for `--max-inbound-message-size` flag to the Ledger Export tool.

Add cons, find, delete and deleteBy functions to NonEmpty module.
```

CHANGELOG_BEGIN
CHANGELOG_END
adriaanm-da pushed a commit that referenced this pull request Oct 6, 2021
@azure-pipelines
release 1.18.0-snapshot.20211006.8003.0.cfcdc13c (#11134)
deddfd7 
This PR has been created by a script, which is not very smart
and does not have all the context. Please do double-check that
the version prefix is correct before merging.

@adriaanm-da is in charge of this release.

Commit log:
```
cfcdc13 Add deprecation output for sandbox classic (#11119)
ac5c52d DPP-609 Add StorageBackendTestsDeduplication (#11088)
55af7ad HttpServiceTestFixture provides a sandbox-classic ledger used for performance tests (#11128)
63ab3f3 Fix change ID references in services.rst (#11130)
31db15d Port error code changes (#11113)
9fd8182 move reusable functions from ContractsFetch to new fetch-contracts library (#11057)
c2084f6 Try and improve the explanation of compatibility test exclusions (#11114)
4df9b7c Rename command completion dedup time to duration in participant code [KVL-1057] (#10901)
11bc22d LF: builtins to create, signatory, and obersver on interface payload. (#11120)
d4cb1f9 KV ledgers: disable participant side command de-duplication [KVL-1083] (#11095)
b9a5a83 Minor: fix trace logging in `TimeBasedWriteSetSelector` (#11117)
018e908 [JSON-API] Make key_hash idx non unique  (#11102)
429f437 Remove HA indexer integration test on H2 (#11104)
9cffa1f LF: check LF transaction protobuf aginst local snapshots (#11064)
d7ee278 Optional table prefix for trigger service  (#11047)
268b2d3 Command deduplication - test case that uses CommandService and CommandSubmissionService [KVL-1106] (#11098)
fac05f6 bypass value enrichment for the performance test (#11112)
4860271 Port file was not written to... where? (#11108)
a8d703b Increase akka server request-timeout for HttpServiceIntegrationTests (#11109)
37d94aa Add unit for daily performance tests (#11107)
eb4ac8a Increase timeouts for resetting database (#11103)
db75f7d interface methods: Scala Typechecker (#11097)
d3e6f16 [JSON-API] Migrating tests to use sandbox next (#11034)
ffc8d68 Data dependencies deserialize imports (#11037)
4b31bf0 update compat versions for 1.17.0 (#11075)
9fd6326 interfaces: consuming/non-consuming iface choices (#11009)
229ce47 Data dependencies serialize imports (#11038)
5d9ec65 Add test case for parallel command deduplication using mixed clients [KVL-1090] (#11093)
d64d965 kvutils: Print state updates before and after normalization. (#11096)
be216aa update NOTICES file (#11089)
517e866 Extract common code for command dedup conformance tests [KVL-1090] (#11092)
7a4963b add `--max-inbound-message-size` flag to `daml ledger export` (#11087)
26d10b2 Drop NodeId type parameter (#10921)
3cbd922 More NonEmpty functions (#10930)
bcd4686 interface methods: Speedy (#11076)
85adaab [DPP-417][DPP-595] Error codes switching - follow-up (#11074)
d7b280b Remove debug print in daml-lf/interpreter build (#11091)
1ed6428 LF: move archive snapshots in a separate directory (#11081)
5e424f8 Command dedup conformance suites readme (#11051)
8290347 DPP-496 HA indexer integration test (#11033)
735c309 Switch from InputStream to Byte Array at the binary content JDBC transport (#11072)
6ae3afa fix perf reporting (#11073)
bf801a6 Fix SimpleDeduplicationBasic when participant deduplication is disabled (#11083)
62234dc [Short] Remove unused code (#11079)
c1d1521 interface methods: Scala AST (#11070)
7dd9c2d Remove expectations for internal failures from parallel command dedup tests (#11061)
5112599 Explicit discard in daml-lf/interpreter (#11067)
554b36c rotate release duty after 1.18.0-snapshot.20210928.7948.0.b4d00317 (#11059)
d156964 Release SDK 1.17.0 (#11062)
6d8cf70 Handle interface methods in LF conversion (#11054)
87ecf1f daml-ledger: better errors for non-json responses. (#11055)
ec2d26f [Divulgence pruning] Fixes divulgence pruning offset update query (#11046)
f13c6d6 release 1.18.0-snapshot.20210928.7948.0.b4d00317 (#11058)
d1805a3 More dependabot fun (#11063)
07b273a update NOTICES file (#11060)
```
Changelog:
```

- [JSON-API] make key_hash indexes non-unique, this fixes a bug where a duplicate key conflict was raised on the query store when the same contract was being witnessed twice by two separate parties

* [Trigger Service] Enable the new `tablePrefix` setting in the `--jdbc`
  flag to add a prefix to all tables used by the trigger service to
  avoid collisions with other components using the same db-schema.
* [Trigger Service] Enable the new ``--allow-existing-schema`` flag to
  initialize the trigger service on a database with a pre-existing
  schema.

- [Daml Assistant] Add support for `--max-inbound-message-size` flag to the Ledger Export tool.

Add cons, find, delete and deleteBy functions to NonEmpty module.
```

CHANGELOG_BEGIN
CHANGELOG_END

Co-authored-by: Azure Pipelines DAML Build <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cocreature cocreature cocreature approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@leonidr-da @cocreature