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

extractor: subscribe to multiple parties #1360

Merged
merged 15 commits into from May 28, 2019

Conversation

@S11001001
Copy link
Contributor

commented May 23, 2019

The extractor --party option may now specify multiple parties, separated by commas; e.g. instead of --party Bob you can say --party Bob,Bar,Baz and get the contracts for all three parties in the database.

Fixes #1353.

Pull Request Checklist

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.

@S11001001 S11001001 self-assigned this May 23, 2019

@S11001001 S11001001 marked this pull request as ready for review May 24, 2019

@S11001001 S11001001 requested a review from bethaitman as a code owner May 24, 2019

@leo-da

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

@S11001001 I will give it a more thorough review on Tuesday... but so far I don't see changes in the com.digitalasset.extractor.config.ConfigParser to support multiple parties. It still says it is a String, needs to be Seq[String]

      opt[String]("party")
        .required()
        .action((x, c) => c.copy(party = x))
        .text("The party whose contract data should be extracted.")

I would also make it plural: "parties"

@bitonic

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

@leo-da I think we should keep --party. @S11001001 my preference would be to be able to mention --party multiple times, because you can have spaces in parties.

@bitonic
Copy link
Contributor

left a comment

Looks good, but please add a tests that ensures we can use parties with spaces in them.

@remyhaemmerle-da
Copy link
Contributor

left a comment

Nice.

@leo-da

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

@S11001001 @bitonic I don't think it will work with multiple --party parameters, --party has to be: opt[Seq[String]] to support one or more.

@leo-da

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

@S11001001 @bitonic I don't think it will work with multiple --party parameters, --party has to be: opt[Seq[String]] to support one or more.

actually it has to be unbounded:

  /** Allows the argument to appear multiple times. */
  def unbounded(): OptionDef[A, C] = maxOccurs(UNBOUNDED)
@S11001001

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

my preference would be to be able to mention --party multiple times, because you can have spaces in parties.

@bitonic Not sure how these are related; parties are comma-separated in this PR, not space-separated.

@@ -279,7 +279,7 @@ object ConfigParser {
cliParams.ledgerPort,
from,
to,
cliParams.party,
ExtractorConfig.parties(cliParams.party),

This comment has been minimized.

Copy link
@S11001001

S11001001 May 28, 2019

Author Contributor

@leo-da This is the splitting you were looking for; I have a weak preference for doing as little parsing into CliParams as possible, but do you want me to add moar type safety for CliParams?

This comment has been minimized.

Copy link
@leo-da

leo-da May 28, 2019

Contributor

nah, it is fine... but my main concern was documentation, the help screen. Plus the thing that @bitonic wanted you need unbounded option configuration, to support multiple party options.

This comment has been minimized.

Copy link
@S11001001

S11001001 May 28, 2019

Author Contributor

Docced in 1c2cb89

@S11001001 S11001001 added this to the Maintenance milestone May 28, 2019

@S11001001

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

Looks good, but please add a tests that ensures we can use parties with spaces in them.

@bitonic Added in b78174d; thanks.

@leo-da

leo-da approved these changes May 28, 2019

@leo-da

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

my preference would be to be able to mention --party multiple times, because you can have spaces in parties.

@bitonic Not sure how these are related; parties are comma-separated in this PR, not space-separated.

@S11001001 I think @bitonic wants to support spaces in the Party names like:

--party "Party A with spaces" --party "Party B with spaces"
or maybe:
--party "'Party A with spaces','Party B with spaces'"

@S11001001

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@leo-da I understand that, but as you noted --party "Party A with spaces,Party B with spaces" suffices for that, so I'm not sure why spaces-in-parties entails passing the --party option multiple times.

@bitonic

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

@S11001001 I find spaces + commas quite awkward. in any case, this is ok for now.

@S11001001 S11001001 requested a review from leo-da May 28, 2019

@S11001001

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@leo-da I'd appreciate a re-check as I've changed the design to be closer to what you might have been looking for, so we have better command-line error reporting (doesn't just pass malformed parties through to API). I've also created an exciting new merge conflict for your #1361 🎉

@leo-da

leo-da approved these changes May 28, 2019

Copy link
Contributor

left a comment

LGTM

@S11001001 S11001001 merged commit fd02a91 into master May 28, 2019

7 checks passed

Summary 2 potential rules
Details
digital-asset.daml Build #20190528.96 succeeded
Details
digital-asset.daml (Linux) Linux succeeded
Details
digital-asset.daml (Windows) Windows succeeded
Details
digital-asset.daml (macOS) macOS succeeded
Details
digital-asset.daml (perf) perf succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@S11001001 S11001001 deleted the 1353-extractor-multiple-parties branch May 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.