-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Use chaining for all Builder parameters #508
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
Conversation
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 is defintely more consistent
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.
Could you keep it as it were? Thx
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.
Sure, I'll bring this back in my amended commit.
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.
any specific reason to delete this?
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.
My interpretation of the pattern was that ExecutionStrategyParameters had access to the constructors of Builder, while outside classes do not. Given that, ExecutionStrategyParameters#transform(Consumer) could call the constructor directly instead of via an additional layer of indirection (the static accessor of the constructor). There were no external callers of ExecutionStrategyParameters#newParameters(ExecutionStrategyParameters), so it was safe to delete.
What's the reason for using this 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.
yes, you are right: it would be nicer to just call the copy constructor directly and remove this method.
7e7aa5e to
63a333f
Compare
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.
In general I like this PR ...thank you!
See my comments about final for a change I would like to see.
It is a bit hard to review because of the reordering of code. As a general suggestion: I think it would be better to split up changes and reordering in the future.
Thanks again
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.
could you remove the final initialiser for the arguments? also for the other methods? thanks
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.
Removed in f66494b
63a333f to
f66494b
Compare
|
I tried to fix this up as a maintainer but I cant push upstream to the originating git My git fu is not super strong I have created #524 which is this with the conflcts resolved |
Do you want this change? In summary, it maintains the
Foo.Builder newFoo()pattern but moves any parameters passed tonewFoomethods to chaining methods.