-
-
Notifications
You must be signed in to change notification settings - Fork 67
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
WIP: Annotations Reloaded for 10.0 #597
base: dev/annotations-reloaded
Are you sure you want to change the base?
WIP: Annotations Reloaded for 10.0 #597
Conversation
Split off integration tests to their own module Added mockito junit5 unit tests
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.
This is kinda my initial review which consists only of static code review, I did not test any of this.
For a more thorough review, I would definitely want to run and test this code myself first.
NamespacedKey id; | ||
|
||
@Subcommand("players") | ||
public void players(CommandSender sender, |
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.
Should have the @Executes
annotation
Same with a lot of these executor methods.
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.
From reading the spec doc, I'm not entirely clear on the purpose of @Executes
over @Subcommand
or why both would need to be included. What does this actually mean and how should it affect the generated code?
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.
Well, @Executes
is supposed to mark a method as executable.
The usage of @Subcommand
can change a bit. You can either use this multiple times on an inner class to not create too many nested classes, or you actually do all those nested class and put one @Subcommand
per inner class.
Or, in cases like this one, you can omit the inner class and put the @Subcommand
annotation on the method - alongside @Executes
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.
Thinking about what is nicest for the user then, I think the @Executes
on this method can be inferred from the presence of @Subcommand
. The intention is clear, so it'd be better if this just works rather than requiring them to add an extra annotation to make it work.
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.
Yeah, but I don't think it should be that way since the way the @Subcommand
annotation is used here is syntactic sugar for this
@Command
public class Blah {
@Subcommand
public class BlahSub {
@Subcommand
@Executes
public void executorMethod(Player player) {}
}
}
instead of this:
@Command
public class Blah {
@Subcommand
public class BlahSub {
@Subcommand
public class BlahSubSub {
@Executes
public void executorMethod(Player player) {}
}
}
}
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 feel like I am missing something. Yes, it's syntactic sugar. I don't see why that means we should make the user add an extra annotation that can be inferred. What problem does this cause?
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.
No, my point is that because it's syntactic sugar we should require the extra @Executes
annotation.
I don't know, maybe that's just my personal preference or maybe I just want to stick to the spec.
Maybe we just need other opinions. I think I've stated mine.
/** | ||
* @return The full description for this command's help | ||
*/ | ||
String 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.
Rename this field accordingly - fullDescription()
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.
'value' is a special property for Annotations. It allows the user to omit the property name, e.g. @Help("Does a thing")
. I didn't write this, but I'm guessing this is why @JorelAli wrote it this way. I don't know if this is desirable though.
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 stuff like help where you have multiple values it'd be definitely better to properly name all the fields imo.
*/ | ||
@Target(ElementType.TYPE) | ||
@Retention(RetentionPolicy.SOURCE) | ||
public @interface Help { |
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.
Help supports usage. This is missing from this annotation
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.
That'd be good to support. There are probably a lot of details like this it'd be good to make a note of so they can all get addressed. This is really only half finished as yet, I just wanted to get some feedback on the general structure and approach before writing every little detail in this style.
...eloaded/src/main/java/dev/jorel/commandapi/annotations/reloaded/annotations/Subcommands.java
Show resolved
Hide resolved
@Primitive("org.bukkit.block.Biome") | ||
@Retention(RetentionPolicy.SOURCE) | ||
@Target({ElementType.PARAMETER, ElementType.FIELD}) | ||
public @interface ABiomeArgument { |
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.
This annotation should be a class with inner annotations @ABiomeArgument.Biome
and @ABiomeArgument.NamespacedKey
since this argument supports using a NamespacedKey variant.
...notations/reloaded/modules/subcommands/RuleTopLevelSubCommandClassesMustHaveASuperClass.java
Outdated
Show resolved
Hide resolved
...pi/annotations/reloaded/modules/subcommands/RuleTypeSubCommandsCanOnlyGoOnNormalClasses.java
Outdated
Show resolved
Hide resolved
public interface AnnotationParser< | ||
AnElement extends Element, | ||
AParserContext extends AnnotationParserContext<AnElement>, | ||
AReturnType | ||
> { |
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.
Put this on one line
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.
Are you sure? It's going to make for some very long lines.
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.
Yeah, I know. If you are not too fond of this, at least put the closing angular bracket and the opening curly brace on the line above. Because this looks weird.
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.
There are some examples of a bunch of generics like this in other classes. For example, AbstractArgumentTree
:
CommandAPI/commandapi-core/src/main/java/dev/jorel/commandapi/AbstractArgumentTree.java
Lines 14 to 22 in aacd957
public abstract class AbstractArgumentTree<Impl | |
/// @cond DOX | |
extends AbstractArgumentTree<Impl, Argument, CommandSender> | |
/// @endcond | |
, Argument | |
/// @cond DOX | |
extends AbstractArgument<?, ?, Argument, CommandSender> | |
/// @endcond | |
, CommandSender> extends Executable<Impl, CommandSender> { |
Honestly, a bit wonky too, so maybe there's a better style we want to establish consistently across the classes.
Also, iirc, the @cond DOX
thing is around the generics like that to fix something with how Doxygen displays the javadocs (see e6f7796)? In this case, that would apply to the AParserContext
type:
AParserContext
/// @cond DOX
extends AnnotationParserContext<AnElement>
/// @endcond
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 it's best to use the same style as the rest of the project here then for consistency, and if we want to change it, we can have an issue to do that across the whole project.
* Apparently rawtype warnings are unavoidable here. See | ||
* https://stackoverflow.com/questions/69491202/how-to-use-sealed-classes-with-generics | ||
*/ | ||
public interface ISuggestions { |
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.
Should be renamed to Suggestions
instead of ISuggestions
.
/* ANCHOR: warps */ | ||
/* ANCHOR: warps_command */ | ||
@Command("warp") | ||
public class DocumentationWarpCommand { |
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.
In general, the anchor names should be the name of the documentation page that they appear in. The first example would be something like page_name1
or pageName1
and so on.
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 have no clue how the documentation pages work, however everything will need to be properly documented before dev/annotations-reloaded
can be merged into dev/dev
for 10.0. We should probably create an issue for doing this, it will need to be the last thing we do once the full annotation spec is finalised.
For this PR, dev/annotations-reloaded
<- dev/annotations-reloaded
, I think we should focus on making sure this is the base we want to build upon for the new annotations system. We can then create all the issues we need for missing features and improvements, and go at it piecemeal from there.
*******************************************************************************/ | ||
package dev.jorel.commandapi.annotations.reloaded; | ||
|
||
import javax.annotation.Nonnull; |
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.
In general, we assume that all code written in the CommandAPI repository has methods that return non-null values, unless explicitly declared with @Nullable
. Under this assumption, this annotation is irrelevant.
Relating to Annotations Spec Doc