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

[RFC] feat: Middlewares addition #263

Merged
merged 3 commits into from
Oct 17, 2022
Merged

Conversation

petarvujovic98
Copy link
Contributor

@petarvujovic98 petarvujovic98 commented Oct 13, 2022

Add middlewares decorator
Add middlewares test

Keep in mind that right now the middleware decorator needs to be applied after the other function decorators to preserve the capturing of the respective methods. This, however, could be changed if we want to change the way we capture the decorated methods, e.g. by adding a brand property to the decorated functions rather than looking for the name of the function decorator.

Addresses #256

Add middlewares decorator
Add middlewares test
Make `Middleware` a function interface
Add multiple middleware functions test
@ailisp
Copy link
Member

ailisp commented Oct 14, 2022

This is great implementation! It's using a different approach as in #256, but achieves same semantics!

@petarvujovic98
Copy link
Contributor Author

One thing that I am not satisfied with is type inference in the decorator. It would be ideal if I could get the editor/language server to tell the developer what type the middleware arguments will be when the middleware decorator is attached to a method. I have not yet seen something similar, but I will try to come up with a solution.

@ailisp
Copy link
Member

ailisp commented Oct 17, 2022

One thing that I am not satisfied with is type inference in the decorator. It would be ideal if I could get the editor/language server to tell the developer what type the middleware arguments will be when the middleware decorator is attached to a method. I have not yet seen something similar, but I will try to come up with a solution.

This sounds challenging. I think we can address in future PRs. Your current middleware implementation is general, easy-to-use and well tested. IMO, it's ready for merge.

@volovyks Do you observe any issue regarding Petar's implementation?

Add proper logging to output shown when verbose flag is active
Copy link
Collaborator

@volovyks volovyks left a comment

Choose a reason for hiding this comment

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

No objections @ailisp , it is a great implementation.

@volovyks volovyks merged commit 04717e5 into near:develop Oct 17, 2022
@petarvujovic98 petarvujovic98 mentioned this pull request Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants