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

API additions + changes for Configurate 4 #2193

Merged
merged 6 commits into from
Nov 15, 2020

Conversation

zml2008
Copy link
Member

@zml2008 zml2008 commented Jul 28, 2020

SpongeAPI | SpongeCommon

This updates the API for changes in Configurate, and to move away from the Guava TypeToken.

API spec changes

The biggest non-obvious changes are to injection -- since these are defined in the implementation they aren't captured super well. I've added in a few new injectable types:

  • ConfigurationReference<CommentedConfigurationNode>: both shared and private roots, provides an auto-reloading configuration
  • TypeSerializerCollection: Exposes the Sponge-defined serializers

and set up some customized default options for configurations:

  • implicit initialization is enabled for object-mapped values
  • defaults are automatically copied

@ImMorpheus ImMorpheus added the api: 8 (u) version: 1.16 (unsupported since Oct 17th 2023) label Aug 29, 2020
@zml2008 zml2008 force-pushed the api8/configurate-4 branch 2 times, most recently from 16ee119 to 9e873c6 Compare October 14, 2020 01:04
@zml2008
Copy link
Member Author

zml2008 commented Oct 14, 2020

Alright, I've made some more progress on this PR, and most of the groundwork is in.

The big change (apart from Configurate 4 itself) is moving away from Guava's TypeToken. While performing that migration, I've also been moving the API to align with the pattern I've begun using in Configurate, which avoids a lot of unnecessary TypeToken usage.

For areas where a type is expected from a user, two variants are provided: One that accepts a TypeToken<T> and one that accepts a Class<T>. For areas where a type is provided to a user, a plain Type is provided. In most cases, the actual value of the T parameter in a TypeToken is not actually known at runtime, and so it provides a false sense of type-safety. This leaves the TypeToken as a tool for type capture more type storage. For simple cases, this means that users don't have to provide a TypeToken when one is not necessary to fully express their type. For example, most of the test plugins in common barely need TypeToken. Some areas affected quite a bit by this change are the key-type objects in API - Key, Parameter.Key, and EventContextKey, and the API that works with them.

Where this strategy gets a bit tricky is for methods that return TypeToken in interfaces designed to be implemented by users. I think most of those will have to stay, since we can't have two options for return types on the method, and in those cases the type parameter is needed to prevent errors -- for example in CommandRegistrar

An area that will need a bit more thought is the catalog registry. While a lot of its API takes TypeToken, internally it still uses Class, which results in a bit of a nasty implementation. I don't know if it makes sense to move the internal storage over to Type and make the system generic-aware, or to make sure all the access and registration methods are paired.

With these changes I hope to constrain the spread of TypeTokens to only types that need to capture generic parameters, which will help reduce the verbosity of the API. The majority of the constants in TypeTokens should become irrelevant.

Copy link
Contributor

@dualspiral dualspiral left a comment

Choose a reason for hiding this comment

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

Looks fine from the commands end (does on Common too, too lazy to post twice).


/**
* Gets the class of the element of the {@link Value} this {@link Key}
* is representing. On occasion, if the element is a {@link Collection} type,
* one can occasionally use {@link TypeToken#resolveType(Type)} with
* {@link Class#getTypeParameters()} as the type parameter of a collection
* one can occasionally use {@link ParameterizedType#getActualTypeArguments()} with
Copy link
Contributor

Choose a reason for hiding this comment

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

The javadoc got broken here!

@zml2008 zml2008 marked this pull request as ready for review October 20, 2020 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: 8 (u) version: 1.16 (unsupported since Oct 17th 2023)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants