Skip to content

feat(event-handler): add base router class #3972

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

Merged
merged 17 commits into from
Jul 15, 2025

Conversation

dreamorosi
Copy link
Contributor

Summary

Changes

Please provide a summary of what's being changed

Please add the issue number below, if no issue is present the PR might get blocked and not be reviewed

Issue number: closes #3971


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@dreamorosi dreamorosi self-assigned this May 23, 2025
@boring-cyborg boring-cyborg bot added the event-handler This item relates to the Event Handler Utility label May 23, 2025
@pull-request-size pull-request-size bot added the size/M PR between 30-99 LOC label May 23, 2025
@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label May 23, 2025
Copy link

@dreamorosi dreamorosi assigned svozza and unassigned dreamorosi Jul 9, 2025
@svozza svozza force-pushed the feat/evt_handler_base_router branch from ea7f951 to 7366df9 Compare July 13, 2025 23:09
@pull-request-size pull-request-size bot added size/L PRs between 100-499 LOC and removed size/M PR between 30-99 LOC labels Jul 13, 2025
@boring-cyborg boring-cyborg bot added the tests PRs that add or change tests label Jul 13, 2025
@svozza
Copy link
Contributor

svozza commented Jul 13, 2025

I've updated this PR with mkst of the changes that were in scope in #3971:

  • Abstract BaseRouter class with HTTP method decorators ✅
  • Basic route registration interface ✅
  • Support for dynamic routes (:param syntax) ❌ dynamic routing has to be dealt with in the registry, which is implemented in the class that will extend BaseRouter
  • Generic route() method for multiple HTTP methods ❌ route is an abstract method in the Python implementation so I followed that pattern
  • Abstract resolve() method for concrete resolver implementation ✅
  • Basic RouteHandler interface 💹

Note the two requirements not implemented, is there something I'm missing here that would let me ship them in this PR?

@svozza svozza marked this pull request as ready for review July 13, 2025 23:21
Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

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

Hi @svozza, great PR! I won't be reviewing specifics about TS, but I left two comments about the expected experience we want to have in this utility.

@dreamorosi
Copy link
Contributor Author

  • Support for dynamic routes (:param syntax) ❌ dynamic routing has to be dealt with in the registry, which is implemented in the class that will extend BaseRouter

You're right, this should've been in either #4139 or #4140

  • Generic route() method for multiple HTTP methods ❌ route is an abstract class in the Python implementation so I followed that pattern

That's correct, nothing more to do here - it was phrased awkwardly.


Thanks!

Copy link
Contributor Author

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

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

Left a few suggestions for the tests, they should also remove the Sonar findings

@svozza svozza requested a review from leandrodamascena July 14, 2025 21:39
Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all my feedback @svozza!

@dreamorosi
Copy link
Contributor Author

Thank you both - as discussed offline, we'll merge the PR after tomorrow's release.

svozza and others added 12 commits July 15, 2025 08:48
Co-authored-by: Andrea Amorosi <dreamorosi@gmail.com>
Co-authored-by: Andrea Amorosi <dreamorosi@gmail.com>
Co-authored-by: Andrea Amorosi <dreamorosi@gmail.com>
Co-authored-by: Andrea Amorosi <dreamorosi@gmail.com>
Co-authored-by: Andrea Amorosi <dreamorosi@gmail.com>
Co-authored-by: Andrea Amorosi <dreamorosi@gmail.com>
Co-authored-by: Andrea Amorosi <dreamorosi@gmail.com>
Copy link

@dreamorosi dreamorosi merged commit 3d4998c into main Jul 15, 2025
46 checks passed
@dreamorosi dreamorosi deleted the feat/evt_handler_base_router branch July 15, 2025 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
event-handler This item relates to the Event Handler Utility feature PRs that introduce new features or minor changes size/L PRs between 100-499 LOC tests PRs that add or change tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: create BaseRouter with base methods
3 participants