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: simplify 02-client router funcs #6837

Merged
merged 5 commits into from
Jul 17, 2024

Conversation

damiannolan
Copy link
Contributor

Description

The goal of the PR is to reduce the amount of string parsing and duplicate code. Mostly usage of ParseClientIdentifier when accessing the 02-client light client module router.

  • Consolidate two helper funcs for accessing routes to a single func Route() (LightClientModule, error) method
  • Update GetRoute on the 02-client Router to accept clientType only (aligns with has/add funcs)
  • Add MustParseClientIdentifier for usage in 02-client handlers

ref: #6760 (comment)


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against the correct branch (see CONTRIBUTING.md).
  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/).
  • Added relevant godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

@damiannolan damiannolan marked this pull request as ready for review July 16, 2024 07:15
}

// GetRoute returns the LightClientModule registered for the provided client type or false otherwise.
func (rtr *Router) GetRoute(clientType string) (exported.LightClientModule, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any benefit in keeping this around? Can only see it meaningfully being used in Route? Should be able to inline logic there from looks of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The routes map is unexported so this is just a dumb accessor. But indeed, it's only really being used from Route, but I don't think its a big deal

Copy link
Contributor

@crodriguezvega crodriguezvega 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. Nice refactor!

Copy link

@damiannolan damiannolan added the priority PRs that need prompt reviews label Jul 17, 2024
Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

green < red = good refactor

@damiannolan damiannolan added this pull request to the merge queue Jul 17, 2024
Merged via the queue into main with commit 9bccf22 Jul 17, 2024
67 of 69 checks passed
@damiannolan damiannolan deleted the damian/refactor-get-route-clients branch July 17, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority PRs that need prompt reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants