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
FORGE-2745: Allow customization of java code decoration via delegation. #611
Conversation
Any tests? Also, it would be nice to use the same code formatter. See CONTRIBUTING.md |
Will do for the code formatter. Sorry about that. As for tests, I'll try to add some. |
{ | ||
private UIInput<String> targetPackage; | ||
private UIInput<String> named; | ||
private UIInput<Boolean> overwrite; | ||
private UIInput<String> extendsType; | ||
private UIInputMany<String> implementsType; | ||
|
||
private JavaSourceDecorator<SOURCETYPE> delegate = this; | ||
|
||
public void setJavaSourceDecorator(JavaSourceDecorator<SOURCETYPE> delegate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be protected I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nevermind, I see why you made it public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you want your new decorator implementation to keep the original behavior? Perhaps pass "this" as a parameter to the JavaSourceDecorator functional method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding passing this
, I used to have it in the parameters but that complicates things. For example, in the RestNewEndpointCommand
case, this
at the calling site would be an AbstractJavaSourceCommand
but the decorator itself needs it to be a RestNewEndpointCommand
, which would force users to downcast… That said, I already need to downcast when I instantiate the decorator in my code (I pass this
to my decorator constructor) but, at least, I don't need to deal with it anymore in the decorator itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments, but overall it looks good
import org.jboss.forge.addon.ui.context.UIExecutionContext; | ||
import org.jboss.forge.roaster.model.source.JavaSource; | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Javadoc?
Ok to test |
Retest this please |
@@ -0,0 +1,16 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a license header
Brilliant, loved the decorator strategy and delegation to itself |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added more comments, please review
Added JavaSourceDecorator interface which can be used in AbstractJavaSourceCommand as a delegate, the default delegate being the command itself since AbstractJavaSourceCommand implements this new interface.
Added documentation and tests. Reformatted using the proper formatter. Please review @gastaldi |
Thank you! Good job |
…n. (forge#611) Added JavaSourceDecorator interface which can be used in AbstractJavaSourceCommand as a delegate, the default delegate being the command itself since AbstractJavaSourceCommand implements this new interface.
Added JavaSourceDecorator interface which can be used in
AbstractJavaSourceCommand as a delegate, the default delegate being the
command itself since AbstractJavaSourceCommand implements this new
interface.