Skip to content

Conversation

@tklovett
Copy link
Contributor

Do you want this change? In summary, it maintains the Foo.Builder newFoo() pattern but moves any parameters passed to newFoo methods to chaining methods.

Copy link
Member

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

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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.

@tklovett tklovett force-pushed the builder-consistency branch 2 times, most recently from 7e7aa5e to 63a333f Compare June 19, 2017 15:45
Copy link
Member

@andimarek andimarek left a 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

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in f66494b

@bbakerman
Copy link
Member

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

@bbakerman bbakerman closed this Jun 22, 2017
@andimarek andimarek modified the milestone: 4.0.0 Jun 23, 2017
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.

4 participants