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

Add dry-run support #129

Merged
merged 10 commits into from
Nov 21, 2023
Merged

Add dry-run support #129

merged 10 commits into from
Nov 21, 2023

Conversation

td202
Copy link
Contributor

@td202 td202 commented Oct 6, 2023 •
edited
Loading

Purpose

Expose the dry run GRPC endpoint in the API.

Changes

  • Implement a DryRun struct that encapsulates a dry-run session and exposes each of the operations in a convenient fashion.
  • A simple example using the dry run endpoint.

Checklist

  • My code follows the style of this project.
  • The code compiles without warnings.
  • I have performed a self-review of the changes.
  • I have documented my code, in particular the intent of the
    hard-to-understand areas.
  • (If necessary) I have updated the CHANGELOG.

@td202 td202 requested review from limemloh, abizjak and MilkywayPirate and removed request for limemloh October 11, 2023 14:04
@td202 td202 marked this pull request as ready for review October 11, 2023 14:04
limemloh
limemloh reviewed Oct 13, 2023
View reviewed changes
Copy link
Contributor

@limemloh limemloh left a comment

Choose a reason for hiding this comment

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

Super cool feature, I have some comments/questions to the API though

src/v2/dry_run.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Show resolved Hide resolved
examples/v2_dry_run.rs Outdated Show resolved Hide resolved
MilkywayPirate
MilkywayPirate reviewed Oct 17, 2023
View reviewed changes
Copy link
Contributor

@MilkywayPirate MilkywayPirate left a comment

Choose a reason for hiding this comment

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

Looks good!

My only concern, is that already mentioned regarding the "double awaits".
believe that the "double" awaits could lead to confusion, and I think we should think twice, if that is the API we want to expose to a user.

src/v1/generated/concordium.rs Outdated Show resolved Hide resolved
examples/v2_dry_run.rs Show resolved Hide resolved
limemloh
limemloh reviewed Oct 18, 2023
View reviewed changes
examples/v2_dry_run.rs Outdated Show resolved Hide resolved
abizjak
abizjak suggested changes Oct 18, 2023
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
examples/v2_dry_run.rs Outdated Show resolved Hide resolved
src/v1/generated/concordium.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Outdated Show resolved Hide resolved
examples/v2_dry_run.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Show resolved Hide resolved
@td202 td202 force-pushed the dry-run branch 2 times, most recently from 0d27908 to 15cdcbc Compare November 2, 2023 12:34
abizjak
abizjak approved these changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@abizjak abizjak left a comment

Choose a reason for hiding this comment

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

Please merge main properly first so that the changelog reflects reality.

CHANGELOG.md Show resolved Hide resolved
src/v2/mod.rs Show resolved Hide resolved
examples/v2_dry_run.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Show resolved Hide resolved
src/v2/dry_run.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Outdated Show resolved Hide resolved
src/v2/dry_run.rs Show resolved Hide resolved
@abizjak
Copy link
Contributor

abizjak commented Nov 16, 2023

@td202 Can you please address comments and get this merged?

@td202 td202 merged commit 562cda6 into main Nov 21, 2023
3 checks passed
@td202 td202 deleted the dry-run branch November 21, 2023 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MilkywayPirate MilkywayPirate MilkywayPirate approved these changes

@limemloh limemloh limemloh approved these changes

@abizjak abizjak abizjak 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.
4 participants
@td202 @abizjak @limemloh @MilkywayPirate