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

refactor(swarm): simplify DialOpts #3335

Merged
merged 7 commits into from
Jan 19, 2023
Merged

refactor(swarm): simplify DialOpts #3335

merged 7 commits into from
Jan 19, 2023

Conversation

thomaseizinger
Copy link
Contributor

Description

Instead of tracking an inner enum, move all fields of Opts into DialOpts, setting them directly once we construct them. This makes the getters a lot simpler to implement and reduces code duplication.

Notes

Helps with implementing #3328 (comment).
Draft until #3318 is merged because it is stacked on top of it.

Links to any relevant issues

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Instead of carrying around an enum and parsing its contents later,
directly set the correct values as we construct the final `DialOpts`.
Base automatically changed from no-run-title-as-command to master January 17, 2023 22:13
@thomaseizinger thomaseizinger marked this pull request as ready for review January 18, 2023 00:58
@thomaseizinger
Copy link
Contributor Author

Given that this isn't super trivial, I'll wait for #3331 to be merged before sending this.

@thomaseizinger
Copy link
Contributor Author

Inrerop tests aren't required yet but are green!

@mergify mergify bot merged commit d82c2a1 into master Jan 19, 2023
@mergify mergify bot deleted the 2824-refactor-dial-opts branch January 19, 2023 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants