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

feat(reflect): added support for static methods reflection #1023

Merged
merged 10 commits into from
Dec 16, 2022

Conversation

pierissimo
Copy link
Contributor

@pierissimo pierissimo commented Sep 29, 2022

Changes included

Add support for the reflection of static methods in a class.

#1022

@pierissimo pierissimo marked this pull request as ready for review September 29, 2022 13:54
@ktutnik
Copy link
Collaborator

ktutnik commented Dec 5, 2022

Sorry for the delay, finally I can cleanup all the test errors on the master branch.

First, I see possible breaking change if I apply this.

Currently now Plumier scan for kind: Method for each action under the controller, with this feature. If any of the controllers have some static methods it will be identified as a route handler (action), this behavior would break the existing functionality.

I suggest that it's better to use kind: StaticMethod and remove the isStatic property, what do you think?

Second, I propose to move the test under tests/behavior/reflect/class.spec.ts and create more scenarios:

  1. Should be able to inspect class static method (test a static function without parameters and return void)
  2. Should be able to inspect static method's parameters
  3. Should be able to inspect static metho's return type.

Big thanks for your contribution.

@pierissimo
Copy link
Contributor Author

hey @ktutnik, I'll implement the changes you mentioned.

Following the same model, even the support for StaticProperties could be introduced in the future.

@ktutnik
Copy link
Collaborator

ktutnik commented Dec 10, 2022

@pierissimo I investigate this feature deeply and found that its more complex than I thought. Then I decided to collaborate on this, and refactor some of my old code to make it cleaner.

I added some abilities, besides the features you added.

  1. Ability to reflect static method without decorators.
  2. Ability to distinguish static vs instance with the same name
  3. Ability to reflect static property & getter and setter

I invited you as an outside collaborator, if you have time please help to review my code.

@ktutnik ktutnik linked an issue Dec 10, 2022 that may be closed by this pull request
@pierissimo
Copy link
Contributor Author

@ktutnik this is awesome, thank you for the help.
Of course, I'll have a look at the changes and provide feedback!

Copy link
Contributor Author

@pierissimo pierissimo 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 to me

@ktutnik ktutnik merged commit f8ee76d into plumier:master Dec 16, 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.

Static methods support for @plumier/reflect
2 participants