Make catalog type builders consistent.#1870
Conversation
| public interface CatalogBuilder<C extends CatalogType, B extends ResettableBuilder<C, B>> extends ResettableBuilder<C, B> { | ||
|
|
||
| /** | ||
| * Sets the name of the {@link CatalogType}. Defaults to {@link #id(String)}. |
There was a problem hiding this comment.
Do not default to the id. Internally you can do it but i would only do it for backwards compatibility.
There was a problem hiding this comment.
The id passed in here is without the namespace/plugin id, so if you provide my_id with id(String), you would end up with a catalog type having my_plugin_id:my_id as id and my_id as name. If it's really desired to have a readable name, then I will enforce the name.
| * @param translation The name translation | ||
| * @return This builder for chaining | ||
| */ | ||
| B name(Translation translation); |
There was a problem hiding this comment.
I'm not sure on the name here. Because on the server the name is something static usually english. However the translation is something dynamic. Maybe rename it to translation and explicitly state that the english content will be used for the name as well.
There was a problem hiding this comment.
Some CatalogTypes are Translatable, this would also apply this translation, and the plain version would be used as getName value.
There was a problem hiding this comment.
Thats why translation might be an alternative.
There was a problem hiding this comment.
Translarion extends Text, iirc, why just not Text?
There was a problem hiding this comment.
Translation doesn't extend Text, but TranslatableText does.
| * | ||
| * @return The built catalog type | ||
| */ | ||
| C build(); |
There was a problem hiding this comment.
Maybe add some throws declarations.
ISE: Mandatory fields not set or no plugin container on the cause stack or run during the wrong game phase.
|
|
||
| /** | ||
| * Builds the {@link CatalogType} of type {@link C}. | ||
| * <p>The last {@link PluginContainer} in the cause stack will be used to |
There was a problem hiding this comment.
Missing empty line before the <p> and missing closing tag.
Cybermaxke
force-pushed
the
consistent-catalog-builders
branch
from
3f227dd to
392f000
Compare
gabizou
left a comment
gabizou
left a comment
There was a problem hiding this comment.
Looks good. I like the extension and this is backwards compatible! Good changes all around.
…eady a catalog type. Don't allow duplicate catalog types. A few documentation related changes. Strip the plugin id when the old manipulatorId method is used.
Cybermaxke
force-pushed
the
consistent-catalog-builders
branch
from
4792ddb to
d40b52e
Compare
6 participants
API | Common
Adds a common builder interface for catalog types to ensure consistency.All original build methods are deprecated and will be removed in bleeding.
Already makes
FurnaceRecipea catalog type. For stable, if no id is set, an auto generated one will be used instead.Please let me know if I missed any catalog type builders. And is it worth to also target stable?
Impl will follow later.