Make static methods that create objects distinct from those that create their builders.

[dualspiral]

This issue is taken from SpongePowered/SpongeAPI#2275 (comment)

One large pain point that I've encountered is that there are factory methods that are named the same as their builder methods. Coupled with the convenience methods on the immutable variants, the method that should be used to create an object is extremely unclear (another issue will be opened on the convenience methods).

Take the following two methods from your library in isolation - what someone booting an IDE and typing Component.text( might see.

Component.text();
Component.text(String);

They both look like factory methods to me: my assumption is that they would both create Components, one empty, one containing the string. However, it turns out that the first is a TextComponent.Builder, and one is a TextComponent. What's bad, however, is that my assumption can actually hold - if I use these as ComponentLikes in something that accepts ComponentLikes, then I will indeed end up with Component.empty() (a method that's actually okay but totally eclipsed by this) and my text component. So now we have two methods that don't do the same thing, but can be treated as such, but are actually totally different ways to do the same thing.

While both can actually be used in the same way, with the same methods names, they really shouldn't be. In theory, I can do the following:

Component.text().content("Text").color(NamedTextColor.GREEN).style(Style.style().decorate(TextDecoration.BOLD).decorate(TextDecoration.ITALIC).build());
Component.text("").content("Text").color(NamedTextColor.GREEN).decorate(TextDecoration.BOLD).style(Style.style(TextDecoration.BOLD).decoration(TextDecoration.ITALIC, TextDecoration.State.TRUE)); // slighly facetious with the starting method, but it proves a point

and you get the same result - one uses the builders, one does not - and because both the builder and the standard component implement ComponentLike, I can use these within other calls and would be no wiser to the fact they are two totally different objects.

I propose the following:

• Deprecate any static methods that return builders and are named the same as factory static methods that do not return builders.
• Create replacement methods suffixed with Builder to clearly mark that those methods return a different type.

[kashike]

I can agree with this - we can change final TextComponent.Builder builder = Component.text() into final TextComponent.Builder builder = Component.textBuilder().

[mbax]

Distinct naming seems like a reasonable solution to resolving potential user confusion. 👍

[kashike]

So one thought I had today - introduce a new mechanism for constructing Components:

interface Components {
  static TextComponent.@NonNull Factory text() {
    return new TextComponentImpl.FactoryImpl();
  }
}

interface TextComponent {
  interface Factory {
    @NonNull Builder builder();

    @NonNull TextComponent of(final @NonNull String content);
  }

  interface Builder {
  }
}

---

Usage:

Components.text().of("foo")
Components.text().builder()

Components.translatable().of("foo")
Components.translatable().builder()

and for those who like to static import:

text().of("foo")
text().builder()

translatable().of("foo")
translatable().builder()

[KingOfSquares]

Personally I like text and textBuilder better, the factory seems like an unnecessary middle step that is also breaking for the current API. Maybe the methods could be moved to their respective component interfaces to get a similar effect?

[dualspiral]

Personally I like text and textBuilder better, the factory seems like an unnecessary middle step...

I would tend to agree - this solution does feel like it's a "please everyone" situation that just adds noise and will end up making everyone go "eh, why?". I know I've been raising these issues but, particularly in the case of #224, it might very well be that you decide that my suggestion does not fit with the library (and that's okay if not, of course - at the end of the day that one in particular is about what I'm thinking). Indeed, I suspect that's where this might be coming from: #224 and trying to de-duplicate code. The thing is, it's so little code to begin with that this is simply adding complexity.

Yes, I personally prefer of and builder methods, but remember they are that, personal preference. I cannot have it my way all of the time. text() and textBuilder() are perfectly good here if you wish to keep static imports - they work and I see no need to upend that. My main issue was really the whole "two methods with the same name are vastly different".

I caution against over-engineering a solution to a problem that could be solved by simply renaming a method. You're never going to please everyone, sometimes we all have to suck it up - just be mindful of what people want!

that is also breaking for the current API.

I'd imagine that this would end up being an Adventure 5 thing.

[lucko]

I've talked about this a bit internally but thought it would be best to reply here too:

I'm against this change.

Firstly, having nice/short names for these methods makes code look significantly nicer.
I've written thousands of lines of components using the current API - trust me the extra characters add up...

I don't agree that the current layout/names is confusing or unclear.

If we really do think it's necessary to create more of a distinction, then I would prefer to see a ComponentBuilders class re-introduced for use in static imports, or go the other way - move the constructor methods somewhere else and leave the methods that return Builders in Component.

Using the builders is the preferred way to construct large components with children or complex properties. I am worried that by changing this, people will fallback to using Component.text(...) etc (which returns a normal, fully built component) and use the mutate methods on those instead.

[kashike]

There does not appear to be a way to make everyone happy, it seems - some people are against changing this (keeping both builder and non-builder methods named the same), while other people are for changing this (appending Builder to the builder methods, for example).

I do not think that a separate class for builders (ComponentBuilders) is going to help much with discoverability - in fact, we used to have the exact same class but it was removed as nobody used it.

I am not sure how to proceed, and I would like to hear any thoughts if anyone has any.

[astei]

I feel like this issue is an unhelpful reproduction of #159, but in the opposite direction. But it more generally raises issues for a lot of our projects.

Before I continue, I'd like to make a digression. In the issue @dualspiral linked, he talks about the disparity between Sponge's API design choices and Adventure's. This would be a valid issue if it weren't for the fact that Adventure is not Sponge. It is used by several major consumers in the Minecraft ecosystem: Geyser, Velocity, Sponge, and eventually Paper. Having Adventure cater more to the needs of the Sponge project hurts the Adventure project and directly affects everyone who is not Sponge who depends on Adventure. (If anything, at the time of writing this, Geyser may be the single biggest user of Adventure in the community). We can bikeshed about how the Adventure API should cater to its individual users all day long and we would not get anywhere.

I ultimately believe that the builder methods aren't even the main complaint - it's the documentation. It's hard to feel motivated to write good documentation. It's not as fun as working on something new and shiny. It is grunt work, and it's something we (especially the improvisational, thinking types like me) try to avoid.

But anyway... back to the issue. I can understand why Component.text() having overloads that return both components and component builders can be confusing. This is worsened by the fact that component builders and components can be used in almost identical ways, but components being immutable and component builders being mutable. Perhaps it's a case of being lucky and getting the result you expected anyway, and if that's the case I could see a potential case for changing the API.

Ultimately, what this issue boils down to is a fight between peoples' personal tastes. APIs are always going to reflect the personal opinions of their authors. It so happens that Adventure values closely mapping how the game treats components, which is not bad but certainly convoluted (I come from the BungeeCord component API which takes a much more intuitive approach than Adventure's 1:1 mapping of vanilla logic). That being said, I do support moving Adventure towards being more discoverable, approachable, and consistent. But I think it's time to determine what standards should be followed before we make any changes.

[DankRank]

Here's something to consider. Renaming the factory methods won't really solve the type confusion problem. The user could create a method which returns a component, and then just forget the return type of that method, and use the return value as if it was a builder (or vice versa). This happened to me a couple of times with text-3. The var keyword makes it even easier to forget what types you're dealing with:

var someText = foo(); // foo returns a component, right?
var someTextButRed = someText.color(NamedTextColor.RED); // I sure hope it does

The core of the issue is that the types are very similar in how they can be used.

However, overall I think things are fine the way they are. The code is more readable this way, and I don't think this should be sacrificed for self-documentation. Instead, documentation proper should be written that describes this idiom and the whole immutability vs. builders thing.

[dualspiral]

I'm glad there is a discussion happening. I don't have much to add about this issue, there are clearly different takes on this and this is a discussion that perhaps should have happened with #159 in the first place. I do want to clarify a couple of things that I think @astei has commented about that need context.

To that end,

Before I continue, I'd like to make a digression. In the issue @dualspiral linked, he talks about the disparity between Sponge's API design choices and Adventure's. This would be a valid issue if it weren't for the fact that Adventure is not Sponge.

Hence why that is a Sponge issue, but it's very important to note that it's also my personal comment. I linked it here for full disclosure. My original intention was to leave my thoughts there and let @kashike pick the parts he felt that he should do something about, but he asked me to post them all on here, so I did.

Let me give a little bit of context as to what happened and so why this issue is here. As we're developing Sponge, some found Adventure hard to use - this was one of the pain points because there was no awareness of immutable text vs builder. One of our core committers, after some discussion, decided to create the commit that in our view, softened the curve for some developers. This ended up with core Adventure developers getting upset with those involved in that commit because they thought that the commit shouldn't exist and that us core committers should PR or discuss things to Sponge before we do anything relating to use of Adventure. After some frank discussion, the commit, in this case, was moved to a PR because @kashike wanted a chance find a way to make us feel better at Sponge in Adventure first. As a result, I felt like I owed it to everyone that I put down my take on that issue.

As such, the point I made about Sponge doing something differently is there in the context of that PR and so it is unfair to call it invalid when I have not made that link here. Choices made in Adventure will directly influence what we do in Sponge, as I would expect in other projects. The gist of it is that Sponge is free to add API endpoints as it feels to access Adventure methods within Sponge, and should feel like they can do so. As it was said that Adventure is not Sponge, the opposite is also true. The same can be said for all other projects that are stakeholders in Adventure. Adventure does not have to do as Sponge does, or other projects do, but they also have to be accepting of the fact that the stakeholder platforms may then do something that they feel supports their users better, whether that be a usability thing they feel Adventure is missing, or a style thing to reduce the learning curve for those who don't need or want cross platform support.

There is merit in rasing these issues in Adventure first, because there might be something that could be added or supported in Adventure to support the issues raised to avoid these platform specific extensions or concerns. As I said, I intended to leave the comment there and let @kashike decide what he wanted to do with it because of the platform specificness of some of the comments. However, he wanted me to post these issues. That's why they are here.

With all that said: I ask that people bear in mind that I made this issue is completely standalone and about usability, I do not make any reference to Sponge in my reasoning - so while yes, I linked that comment here, Sponge is not part of my reasoning for wanting this change, so I think it's important to also bear that in mind.

(For note: If I wanted Sponge style, this would be about bringing back of and builder methods.)

(I come from the BungeeCord component API which takes a much more intuitive approach than Adventure's 1:1 mapping of vanilla logic)

I think you might be looking for #225 there!

That being said, I do support moving Adventure towards being more discoverable, approachable, and consistent. But I think it's time to determine what standards should be followed before we make any changes.

I do have comment on this to talk about developments since this issue was filed pertaining to this.

@kashike and I have been talking a lot in the background about how this could have been better managed in the first place, and something that's recently happened through discussion is the creation of a group in Discord that contains the stakeholders so that changes and discussions that might affect the platforms can be discussed, or even just given. Regardless of the outcome of any of the issues I've posted, which I post in a personal capacity, not Sponge's, this is a win for the project and directly feeds into @astei's points about what the standard is going to be going forward - stakeholders by definition have a stake in the direction of the project and it is sensible that they feel consulted about such things due to the direct impact it will have on them and their projects.

Ultimately, you can not and should not aim to please everyone, but if the goal is to make and keep Adventure as a part of their APIs, indeed potentially replacing APIs such that there is no alternative for that platform, then this is a vitally important step to take.

---

The user could create a method which returns a component, and then just forget the return type of that method, and use the return value as if it was a builder (or vice versa).

Right, a sister issue to this is #223, which talks about the convenience methods on the immutable types. While I talk about suggesting removal of them, I think consensus is to perhaps consider using with as a verb on said methods for immutable types. While I personally would prefer this issue to go through, accepting that would drastically soften the impact of this issue because it becomes a lot more obvious what type you're dealing with.

A lot of those who disagree with this proposal are suggesting that documentation is an issue, and it absolutely, 100% is. Adventure's documentation is currently in a bad state and so there needs to be a lot of effort to clean that up - kudos to @KingOfSquares who took #226 and ran with it - I was going to find some time to do so in the near future when I have a moment. However, I also believe method names are part of that documentation, and I don't think the code is any less readable adding builder as a suffix to a method name, or adding the verb with - but I can see and accept why people don't like it. Code clarity is important too - such a situation that I have in the original comment shouldn't really be able to happen, but both do the same thing so someone who just picks up the library and tries things will end up, potentially, with bad code very quickly.

Consider this then. Use code readability and naming as a way to discourage people from using methods in a way they shouldn't - #223 is a prime example of that. If with is on every method, then the code is "less readable" and people are less likely to use it, less so in a chain.

[kashike]

We will focus on improving documentation (#226, some work in #229) for right now - everyone seems to agree that lacking and/or short documentation is the main problem here.

[nossr50]

I also thought it was weird to get builders from the same named methods giving components, I am all for it being more verbose. It is java after all.

[DankRank]

I think a with prefix is fine, and adding it is sufficient to solve both #223 and this issue (since it'd be easier to tell that the factory methods return different types).

[emilyy-dev]

I personally think a good alternative might be removing (deprecating) Component.text(), Component.translatable() etc and put in place ComponentBuilder.text(), ComponentBuilder.translatable() and so on.
Something as simple as just changing the interface the methods are located in (same names) provides with a different perspective on what is it you're calling actually gives, you know you are calling it in the ComponentBuilder interface, so you have to think "Okay, then this has to do with component builders", seems more intuitive there.