WIP: Annotations Reloaded for 10.0

[XHawk87]

Relating to Annotations Spec Doc

[DerEchtePilz]

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.

[DerEchtePilz]

Should have the @Executes annotation
Same with a lot of these executor methods.

[XHawk87]

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?

[DerEchtePilz]

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

[XHawk87]

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.

[DerEchtePilz]

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) {}
        }
    }
}

[XHawk87]

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?

[DerEchtePilz]

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.

[DerEchtePilz]

Rename this field accordingly - fullDescription()

[XHawk87]

'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.

[DerEchtePilz]

With stuff like help where you have multiple values it'd be definitely better to properly name all the fields imo.

[DerEchtePilz]

Help supports usage. This is missing from this annotation

[XHawk87]

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.

[DerEchtePilz]

This annotation should be a class with inner annotations @ABiomeArgument.Biome and @ABiomeArgument.NamespacedKey since this argument supports using a NamespacedKey variant.

[DerEchtePilz]

Put this on one line

[XHawk87]

Are you sure? It's going to make for some very long lines.

[DerEchtePilz]

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.

[willkroboth]

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

[XHawk87]

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.

[DerEchtePilz]

Should be renamed to Suggestions instead of ISuggestions.

[DerEchtePilz]

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.

[XHawk87]

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.

[JorelAli]

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.