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

Mutable builder list fields #650

Open
wants to merge 9 commits into
base: master
from

Conversation

@ZacSweers
Copy link
Contributor

commented Jun 3, 2018

A case that often comes up for us is that if you expose a post processing API in code gen, builders become a pain because any list property in a builder is append-only.

What this means is that in order to modify one of them, you have to manually make a new copy of the type that builder is constructing and copy over every property but the one you're trying to change. Then for the one you're changing, you have to copy all the other ones and then your modified version.

A classic example is a typespec with a methodspec in it that you want to add an annotation to.

After talking with @swankjesse about it, we decided to take the approach of making builders' collection fields public and mutable with the idea that if you're manipulating those directly, we just assume you know what you're doing and defer to checks in build() to enforce things.

@ZacSweers

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2018

I started moving checks to build(), but wanted to check first - do we want to keep early checks in the main APIs or just move them whole sale to build()?

@ronshapiro

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2018

I don't fully understand the point of this, and my initial instinct is that it's probably going to be abused more often than it is actually useful. I presume, for one, that we wouldn't want someone to call builder.someList.add() and would instead prefer builder.addSomething().

Perhaps you could provide a more real-world example, or point to some code that would benefit from this

@sormuras

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2018

Examples: #341 and #478 and other I don't remember anymore.

One use-case is to build up a type spec and enhance it afterwards with information gathered from that same type spec. Without the need to have an extra meta-model from which you would generate the "final type spec" in one go.

In "beethoven" I called the mutators, components that work on the mutable model, "composers": https://github.com/sormuras/beethoven#composers

@ZacSweers

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2018

Another example in a production library: Thrifty has support for post processing that hands over the TypeSpec. If all you're doing is adding to the top level type, it's sort of fine. If you want to add anything to anything within it that supports multiple instances (methods, fields, annotations, etc), then you run into this problem.

https://github.com/Microsoft/thrifty/blob/master/thrifty-example-postprocessor/src/main/java/com/microsoft/thrifty/compiler/SerializableTypeProcessor.java

@ZacSweers

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2018

Basically - it feels incomplete to have a toBuilder() API that only allows replacement/destructive operations on some of its properties.

@swankjesse

This comment has been minimized.

Copy link
Member

commented Jun 3, 2018

This is great. Do you mind doing a quick validation pass in build()? For example, we shouldn’t have enum constants in TypeSpec unless it’s an enum type?

@ZacSweers

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2018

Moved validation to build() where appropriate, which really only TypeSpec and AnnotationSpec had validations to (or that made sense to) move

@ZacSweers ZacSweers force-pushed the ZacSweers:z/mutableAdditiveBuilderFields branch from 7ef29da to 66ae277 Aug 7, 2019

@ZacSweers

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

I've rebased this, definitely would like to still see this merged!

@ZacSweers ZacSweers force-pushed the ZacSweers:z/mutableAdditiveBuilderFields branch from 66ae277 to d3b04f3 Aug 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.