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 loop registry #11807

Closed
wants to merge 4 commits into from
Closed

Conversation

cedric-cordenier
Copy link
Contributor

As I was doing some exploration of the loop registry, I found the code to be hard to modify and quick tightly coupled together. This is my attempt to clean some of this up.

In particular, I:

  • Collapsed the command factory into the loop registry: these were invariably getting called one after another so this seemed a no-brainer
  • Separated the handling of GRPCOpts from the registration of the loop itself.

This also makes it easier to add a handle to the loop into the registry at a later date if desirable.

@krehermann Please push back if there's something I'm missing

Copy link
Contributor

I see that you haven't updated any README files. Would it make sense to do so?

jmank88
jmank88 previously approved these changes Jan 18, 2024
krehermann
krehermann previously approved these changes Jan 18, 2024
Copy link
Contributor

@krehermann krehermann left a comment

Choose a reason for hiding this comment

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

looks better. thanks for cleaning it up

@cl-sonarqube-production
Copy link

SonarQube Quality Gate

Quality Gate failed

Failed condition 73.3% 73.3% Coverage on New Code (is less than 75%)

See analysis details on SonarQube

Copy link
Contributor

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 20, 2024
@github-actions github-actions bot closed this Mar 27, 2024
auto-merge was automatically disabled March 27, 2024 00:11

Pull request was closed

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