-
-
Notifications
You must be signed in to change notification settings - Fork 415
Type Properties/Attributes #8165
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
Type Properties/Attributes #8165
Conversation
|
I also acknowledge how bloated this may make the already bloated __Classes classes, so I intend a future PR to go through and separate all the classinfos each into their own classes: |
…s, start parse-time checks for Contains
- allows stateful property handlers - removes use of AnyNamed - removes NameHandler in favor of just using ExpressionPropertyHandler - Avoid exposing silly cast to users when possible in PropertyBaseExpression - fix issue with handling DELETE and RESET changers in PropertyBaseExpression - add support for default toString impl in PropertyBaseExpression
|
cool request |
first pass breaking change, removes `size of itemstack`.
…x, add condition base class
|
Added a hidden config option |
|
#8211 is required for changers to work fully, but is not a critical change and the two can be merged independently. |
…/Skript into feature/type-properties
APickledWalrus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking pretty good. These are just some of my thoughts from a very brief look.
I have some concern over publicly exposing the XClassInfo classes as it kind of prevents us from relocating those classes in the future (as needed). I wonder if it might be better to instead promote obtaining the ClassInfo instance and grabbing the changer from that (and marking those classes as private/internal)
src/main/java/org/skriptlang/skript/lang/properties/Property.java
Outdated
Show resolved
Hide resolved
| * If a conflict does occur, your property will fail to register and return false. You should check if your property | ||
| * has a matching handler with the already registered one. If so, you should be able to use the other property instead of your own. | ||
| */ | ||
| public class PropertyRegistry implements Registry<Property<?>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to implement ViewProvider for this. Would require changes to the existing registration process though (e.g you'll need to register the default properties through an AddonModule)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this when efy suggested this, it felt like it made the code unnecessarily complicated for little gain. Is there a reason for it?
| if (Skript.classExists("net.kyori.adventure.text.Component") && | ||
| Skript.methodExists(Bukkit.class, "createInventory", InventoryHolder.class, int.class, Component.class)) | ||
| serializer = BungeeComponentSerializer.get(); | ||
| if (!SkriptConfig.useTypeProperties.value()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard class might be better for indentation management
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you meant guard clause, I didn't bother since this will just be removed in 2 days anyway, once 2.13 is merged.
| * @param <Handler> the type of the handler | ||
| * @return a new property | ||
| */ | ||
| public static <HandlerClass extends PropertyHandler<?>, Handler extends HandlerClass> Property<Handler> of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the presence of documentation-related fields, I think a builder would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried one, just could not figure out how to get the generics to work. It would always give me XYZHandler instead of XYZHandler<?, ?>.
I agree with not making them public, but as far as the changer I fear that they're mostly wanted prior to classinfo registration, so it wouldn't necessarily be feasible to get the instance instead. |
|
Merging without full approval for testing and API exposure reasons. Functionality is locked behind opt-in config option, so impact on normal users is minimal. Further refinement will take place in PRs to follow. |
* basic implementations for Contains and Named properties * fix rogue find+replace * cleanup and move lookupPropertyInfo to override of get(Class) * Remove necessary usage of toHandlerType * add base handler class * Improve property resolution, add back-compat for AnyNamed, AnyContains, start parse-time checks for Contains * cleaning and reorganization * implement common module * Cleanup, fully implement ExprName, many docs, some other improvements - allows stateful property handlers - removes use of AnyNamed - removes NameHandler in favor of just using ExpressionPropertyHandler - Avoid exposing silly cast to users when possible in PropertyBaseExpression - fix issue with handling DELETE and RESET changers in PropertyBaseExpression - add support for default toString impl in PropertyBaseExpression * re-add type in register() and Script#name() * implement documents plus a bit more cleanup * cleanup entity/player changer usage * add property info itself to related syntaxes docs * Update name's description * update docs, add since * small fixes * Replace AnyAmount/ExprAmount first pass breaking change, removes `size of itemstack`. * Finish replacing AnyAmount, add Empty property, add PropertyBaseSyntax, add condition base class * banish utils class * AnyValued replacement * better errors * some docs * fix PropCondContains with somewhat horrible code that works * final cleanup * final cleanup 2 * Apply suggestions from code review more cleanup Co-authored-by: SirSmurfy2 <[email protected]> * reorder methods + missing continue * Update PropExprValueOf.java * Update JSONGenerator.java * Apply suggestions from code review Co-authored-by: SirSmurfy2 <[email protected]> * requested changes * add config option to enable type properties * Update ExprName.java * Use ChangeInPlace when changing property expressions * Update JSONGenerator.java * Update JSONGenerator.java * better CIP requirement detection * improve error message for missing properties * requested changes
* basic implementations for Contains and Named properties * fix rogue find+replace * cleanup and move lookupPropertyInfo to override of get(Class) * Remove necessary usage of toHandlerType * add base handler class * Improve property resolution, add back-compat for AnyNamed, AnyContains, start parse-time checks for Contains * cleaning and reorganization * implement common module * Cleanup, fully implement ExprName, many docs, some other improvements - allows stateful property handlers - removes use of AnyNamed - removes NameHandler in favor of just using ExpressionPropertyHandler - Avoid exposing silly cast to users when possible in PropertyBaseExpression - fix issue with handling DELETE and RESET changers in PropertyBaseExpression - add support for default toString impl in PropertyBaseExpression * re-add type in register() and Script#name() * implement documents plus a bit more cleanup * cleanup entity/player changer usage * add property info itself to related syntaxes docs * Update name's description * update docs, add since * small fixes * Replace AnyAmount/ExprAmount first pass breaking change, removes `size of itemstack`. * Finish replacing AnyAmount, add Empty property, add PropertyBaseSyntax, add condition base class * banish utils class * AnyValued replacement * better errors * some docs * fix PropCondContains with somewhat horrible code that works * final cleanup * final cleanup 2 * Apply suggestions from code review more cleanup Co-authored-by: SirSmurfy2 <[email protected]> * reorder methods + missing continue * Update PropExprValueOf.java * Update JSONGenerator.java * Apply suggestions from code review Co-authored-by: SirSmurfy2 <[email protected]> * requested changes * add config option to enable type properties * Update ExprName.java * Use ChangeInPlace when changing property expressions * Update JSONGenerator.java * Update JSONGenerator.java * better CIP requirement detection * improve error message for missing properties * requested changes
Problem
One of the most irritating problems that has plagued the Skript ecosystem for ages is the inability for two implementations of the same syntax pattern to exist at the same time. In Skript itself, it forces long, complicated Expr classes that handle 7 different types, and in addons it causes syntax conflicts or awkward naming schemes to avoid any possible conflict with other addons or Skript.
AnyX was somewhat recently introduced to assist in this case, by allowing any type to be converted to a uniform interface class that has a common method to use. This works well, but has little to no parse-time information capabilities. There's no way to check if a type that implements
AnyContainscan contain strings, items, or entities until runtime, when it's basically too late.It's also difficult to handle changers and any non-
get()behavior with AnyX, due to the same reasons of limited parse time information. This can make determiningallowChangeand possible return types near impossible to do accurately.Solution
This PR introduces the concept of Type Properties (name wip, property is already rather prevalent in code). These are very free-form properties that can be registered with Skript using a name and a handler interface. Any ClassInfo can use the
.property()method to declare that they implement this property and how they implement it.This means that Skript, SkBee, and Disky may all have their own implementations of
size of xfor their own types without conflicting. You could dolengths of ("xyz" and {skript-particle-rectangle})with no issue.toggle xcould work for any type from any addon that wants to use that property.Implementing a Property
This system revolves around the
Propertyclass, which is, by itself, just a string and an addon reference. Any addon can register a property, though no two addons can register the same property name. Properties require a "handler" class to be supplied at creation to declare the interface all implementations must follow. This can be as simple as:or something much more complex, with changers or complicated behaviors. There are no restrictions, except for the handler must extend
PropertyHandler<Type>, which is an empty interface that just says "This is a property handler".When a
ClassInfowants to implement a property, they use theClassInfo#property(Property, Handler)method. This adds the provided handler implementation to the class' properties for later use. Each property may only be implemented once on aClassInfo.Alternatively, something as simple as
length ofcould use the existingExpressionPropertyHandlerhandler, which provides basic methods for expression-like properties, such as conversion, acceptChange/change for changer support, and custom return types:Or for changer support:
A similar handler exists for simple conditions, the
ConditionPropertyHandler<Type>.Creating a syntax that uses a property's handler
Up till now, this has been very simple for the user. The more complex part is creating a syntax that takes advantage of these properties to actually do something. For simple property expressions (properties that lead to patterns similar to the actual SPE syntaxes, like
name of x), I provide a relative simple base class to extend:This does require the user to either use the
ExpressionPropertyHandlerhander directly, or have their handler implement it:In return, changers and property checks are handled for you. This is what I believe most properties will use.
Some handlers may require state, for which
newInstance()andinit(Expression, ParserInstance)should be overridden:Similarly, I provide an easily extensible base class for simple conditions:
Note this only works for simple conditions that do not require secondary input.
Custom property syntax implementations
However, this API is very powerful and allows way more outside of that. The power, though, comes at the cost of complexity. Users who want unique properties will have to handle property management, type checks, and evaluation themselves. It's not that bad but it is certainly complex.
To start with, users making custom syntax should consider inheriting
PropertyBaseSyntaxfor standardization reasons. It provides agetPropertyandgetPropertyNamemethods which can be used to determine property and for toStrings regardless of the details of the specific syntax.The main things to handle are:
Determining the relevant properties:
Nearly every property will contain something like this in their init():
These
PropertyBaseSyntaxmethods are meant to assist in, respectively, converting an input expression into one that returns things with the given property and getting all the possible handlers that one could need given the expression's return types.getPossiblePropertyInfosin specific is important, since it returns the most useful part of the api, thePropertyMap.A
PropertyMapis aHashMap<Class, PropertyInfo>with 2 special methods:getHandler(Class)andget(Class).getHandleris the main method you should be using, which returns an appropriate Handler implementation for the given class.get(Class)returns thePropertyInfofor the given class or, if not found, determines the closest class to the given class that has a property in the map already. This lookup is cached, so after the call, a new entry should exist in the map, either to null if no property exists, or to aPropertyInfo. This allows, for example,OfflinePlayerto have a property implemented and forPlayerto also use that property.Using the properties
For example, here's my implementation of
CondContainsusing properties:Implementation details can vary wildly between properties, but
PropertyBaseExpressionis a good example of some of the more complex details a property may encounter.PropCondContainsis a good example of a simpler custom property implementation.Documentation
Any syntax element using properties should have the
@RelatedProperty("property name")annotation added for documentation purposes. Each property requires a short description upon creation, describing what it is meant to represent. Likewise, each implementation on a class info also requires a short description, which should be used to describe exactly what on the type it represents, as well as any relevant info like available changers.Documentation is supported via the JSON docs. An array containing all properties with their related types and syntaxes is provided, and any syntax annotated as @RelatedProperty will have a list of all the classes that implement that property, as well as the info about the property itself. ClassInfos will have their properties listed out with the property id, name, and a class-specific description of how it implements the property, plus a list of related syntax ids.
Testing Completed
Only manual testing has been done so far. Existing tests for implemented classes succeed without issue.
Supporting Information
Completes: none
Related: #7644 #7919 #7675 #7416 #8136