Skip to content

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.

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