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

v8: Make TypeCollectionBuilderBase chainable #5350

Closed
mattbrailsford opened this issue Apr 26, 2019 · 7 comments · Fixed by #5354
Closed

v8: Make TypeCollectionBuilderBase chainable #5350

mattbrailsford opened this issue Apr 26, 2019 · 7 comments · Fixed by #5354

Comments

@mattbrailsford
Copy link
Contributor

When composing collections, most of the CollectionBuilderBase classes are chainable such that you can chain add / remove commands in a fluent manor. For some reason though this doesn't appear to be possible with the TypeCollectionBuilderBase. I think this should be made consistent with the other base classes and allow a similar API.

@zpqrtbnk
Copy link
Contributor

would totally make sense indeed. Would it be a breaking change? Guess yes... 8.1 then?

@mattbrailsford
Copy link
Contributor Author

I don't think so as all Add / Remove methods are currently return void so those can continue to function as before, then just now return a instance of the collection to allow chaining.

@mattbrailsford
Copy link
Contributor Author

ie, yes the signature is changing, but because they didn't return anything before, I think it's ok to now make them return something.

@zpqrtbnk
Copy link
Contributor

fancy a PR? :-)

@mattbrailsford
Copy link
Contributor Author

@zpqrtbnk would love to if I wasn't spending all my time v8-ifying TC 😁

@zpqrtbnk
Copy link
Contributor

PR #5354

@zpqrtbnk zpqrtbnk removed their assignment Apr 27, 2019
@ghost ghost removed the state/review label May 1, 2019
@Shazwazza
Copy link
Contributor

Have merged, it's marked as breaking (since the generic signature changes) but expect this will affect a very very tiny amount of people

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants