Split Treasury's code into modules

[MrIvanPlays]

Intends to close #42

[MrNemo64]

I've checked the code and since the migration logic is the same, it only changes the trigger that starts it, for example the trigger for bukkit/spigot/paper is a bukkit command but for other servers with their own command system the trigger would be their own command; the migration logic could be in the core module.
I'd also rename the Bukkit module into Bukkit-core so you know it has the bukkit plugin logic

[MrIvanPlays]

As I said, this is not a completed pull request. Stuff which can be abstracted are yet to be abstracted. Some general API stuff will probably need to be changed. It is not complete so please do not do any reviews, only suggest stuff to be abstracted.

[Geolykt]

I think that it might be better to split the modules over multiple repos, but that may also be annoying for others

[MrNemo64]

I think that it might be better to split the modules over multiple repos, but that may also be annoying for others

Most repos I see have them all in one place being childs of a common parent.
The problem with having them together is that for the simplest of changes you need to clone everything, that can be anoying
Having them separated doesnt seem like much of a issue to me

[MrIvanPlays]

No. The reason why they're called modules is that you contain them at 1 place on the internet.

[Geolykt]

Then we will run into issues with forge + fabric should we ever choose to support them. It takes ages to compile those

[MrIvanPlays]

Why should we support fabric/forge? Either way they're hassle and if we do so we'd have to switch to using gradle

[Geolykt]

Why should we support fabric/forge? Either way they're hassle and if we do so we'd have to switch to using gradle

Because we fully can. I also believe that having a cross-plattform economy API for once would help immensely.

[MrIvanPlays]

In that case we'd need to migrate to gradle, which is a hassle for multiple modules and lots of other things. But we need @lokka30 on that. Fairly sure we don't need fabric/forge support and gradle.

[Geolykt]

Yeah, right now it is not that good to have that support; was just throwing the idea around

[MrNemo64]

I haven't really seen mods with an economy system thing or that needs an economy provider, they all normally just add the items that they need and not depend on anything else, and if supporting them means switching to Gradle it may be a little to much for so little, I can't think of mods that need an economy interface but I'm not really into mods. It may make more sense to support servers that allow the usage of mods and plugins

[MrIvanPlays]

Mods are to add stuff which vanilla minecraft can't offer or to optimize minecraft's internals. They're not used for such simple additions.

[lokka30]

• I don't believe Treasury will have mod support in the future. Happy to include it if we ever run out of things to do, though. 😄
• I'm against splitting Treasury into multiple repositories. How come would we need to do this? Or is this only for the Forge/Fabric support?
• I very briefly gave an attempt at using Gradle once and it failed miserably. If there are decent benefits with Gradle then we can make the switch. I know it does compile much faster.

[MrNemo64]

• I'm against splitting Treasury into multiple repositories. How come would we need to do this? Or is this only for the Forge/Fabric support?

This is not necesarily only for forge/fabric support. Instead of a parent project and multiple modules we would just have multiple modules that depend on each other

• I very briefly gave an attempt at using Gradle once and it failed miserably. If there are decent benefits with Gradle then we can make the switch. I know it does compile much faster.

The only benefit I see with gradle instead of maven is compile time, but lets be real does it take that much time to compile anyways? If I'm not mistaken Gradle has more bugs, less repositoryes and there are easy ways to use a maven repo on a gradle project but not otherways (may be wrong here). Gradle also tends to be harder to use

[lokka30]

This is not necesarily only for forge/fabric support. Instead of a parent project and multiple modules we would just have multiple modules that depend on each other

Wouldn't the API be a parent module?

I still don't see any reason to split Treasury into multiple repositories - surely, the project's code can't become large enough to justify that?

Edit: Accidentally had 'multiple modules' posted for a few seconds. Hope nobody saw that. 😄

The only benefit I see with gradle instead of maven is compile time, but lets be real does it take that much time to compile anyways? If I'm not mistaken Gradle has more bugs, less repositoryes and there are easy ways to use a maven repo on a gradle project but not otherways (may be wrong here). Gradle also tends to be harder to use

Compile time is a very minor developer luxury - although Maven takes a few seconds longer than I think it should, it's really far from enough to justify moving to Gradle. I assume more people know how to use Maven than Gradle as well.

[MrIvanPlays]

Gradle doesn't have their own repository schema. They use maven's repositories. Benefits of gradle are compile time and that you can make custom actions when compiling - say to validate something (maven can also validate stuff via the plugins, but is kinda limited, so dont make it a solid plus). Also gradle has a lot of plugins and you can make your own plugins for it. Unfortunately, gradle is kind of a hassle when it comes to deploying to a maven repo (it can, but it is buggy), having multiple modules with it is also not really a go-to. We're sticking to maven as it is easy to use.

[Geolykt]

From my experience gradle has been far easier for deployment than maven as latter does not create the checksums. But I am also using a highly unusual publishing system where I push to mavenLocal first and then manually copy them over to the webserver

[MrIvanPlays]

I think we've decided about what build system we will use. End of discussion maven vs gradle.

Now you may want to check out the newest changes and suggest things on the put // todo's where help is needed.

[MrNemo64]

I think we've decided about what build system we will use. End of discussion maven vs gradle.

Now you may want to check out the newest changes and suggest things on the put // todo's where help is needed.

Can you explain a little your thought process?

[MrIvanPlays]

I'm aiming to abstract as much as it can be abstracted into the core module so it is easier to implement treasury on different platforms.

There's a lot to do in here ; I plan to document every part of that core module so it is

1. easy to maintain
2. easy on implementing different platforms
3. easy on someone who wants to do a simple fix

Plan is the following:

1. Get rid of microlib (sorry, lokka30, but microlib's too bukkit oriented), which means to implement :

• platform independent messaging system
• platform independent settings handling
• platform independent update checker

2. Implement platform independent getter for economy provider and provider registrar data.
3. Implement platform independent command system.
4. More stuff to do on the code (can't think of more stuff as of right now)
5. Work on maven :

• deployment profiles for codemc and repo.mrivanplays.com
• compile in the jar only what we need, provide only otherwise.

Some stuff have been implemented or is being worked on, I may create a table on the opening comment to track progress.

[MrNemo64]

• platform independent messaging system
• platform independent settings handling
• platform independent update checker

Do we really need to implement them? The bukkit version of the plugin will check on spigotmc but the paper one may check on papermc and so on. Update checker probably can be on the core module of each platform
The settings handler and messaging system are the ones that seem like they need an abstracted system but mainly so we can have the migration login on the core module.

1. Implement platform independent getter for economy provider and provider registrar data.

I dont understand what you mean here. So in bukkit we have the service provider that would allow to get the economy provider but in other platforms there may not be one so we give the responsability of providing the provider to a new provider?

It may make sense to, before doing mayor changes to the code, discuss what needs to be on what module and what do we need to abstract

[MrIvanPlays]

Do we really need to implement them?

Yes. If we do it per platform then we will have lots of duplicated code, which is something you don't want.

So in bukkit we have the service provider that would allow to get the economy provider but in other platforms there may not be one so we give the responsability of providing the provider to a new provider?

No. See the commands and the other parts of the code use the EconomyProvider class. We need an instance of it in the core module. That's what this means.

[lokka30]

Get rid of microlib (sorry, lokka30, but microlib's too bukkit oriented), which means to implement :

If we add more platforms for Treasury, go ahead, I really don't mind :)

service provider

What if we made our own service provider class, so it can be used universally?

[MrIvanPlays]

I mean, not that we can't, the thing is that we don't really need it. Sure, we'd remove that provider from the core module, but at the cost of maintaining a singleton API.

[MrIvanPlays]

If we're gonna talk speed ; I've just made a simple benchmark of AnnotationConfig if anyone's interested in the numbers. https://gist.github.com/MrIvanPlays/85d248a0e8ab15b71f45106e5359420d

[lokka30]

Please could you show an example of accessing keys in a YAML file?

[MrIvanPlays]

Please could you show an example of accessing keys in a YAML file?

wdym? AnnotationConfig loads the config entries into the object you dump.

private String foo = "bar";

will generate

foo: "bar"

and if the user changes it to

foo: "baz"

it's gonna be the same as:

private String foo = "baz";

so you just access the field with a getter.

[yannicklamprecht]

It's the way you do config normally. BukkitConfiguration is just the worst way of doing config.

[lokka30]

Please could you show an example of accessing keys in a YAML file?

wdym? AnnotationConfig loads the config entries into the object you dump.

private String foo = "bar";

will generate

foo: "bar"

and if the user changes it to

foo: "baz"

it's gonna be the same as:

private String foo = "baz";

so you just access the field with a getter.

Oh, I apologize, I must have had a brainfart whilst asking that question.. Thanks. 😜

[lokka30]

It's the way you do config normally. BukkitConfiguration is just the worst way of doing config.

Do you have any thoughts of Ivan's use of AnnotationConfig in this PR?

[lokka30]

Is there any way we can automate the getters for these - perhaps Lombok?

[MrIvanPlays]

Is there any way we can automate the getters for these - perhaps Lombok?

I don't really like lombok. I mean it is a cool library but it uses hacks. And also the fact a plugin for intellij is required so you can develop normally is just a no-go.
Alt + Insert is much faster and better than lombok.

[lokka30]

Is there any way we can automate the getters for these - perhaps Lombok?

I don't really like lombok. I mean it is a cool library but it uses hacks. And also the fact a plugin for intellij is required so you can develop normally is just a no-go. Alt + Insert is much faster and better than lombok.

I didn't know it required a plugin. A+I it is then 🙂

[MrIvanPlays]

Not really required but if you don't have it you're basically screwed writing that methods manually.

[lokka30]

Not really required but if you don't have it you're basically screwed writing that methods manually.

Regardless it's a developer luxury that can be avoided. I don't think there will be many additions to the config files beyond what is currently there so I don't mind going the manual route - that is, IntelliJ's autocompletion.

[Geolykt]

Lombok will result in the plugin being inaccessible for a lot of potential contributors - avoid it's use whenever possible

[yannicklamprecht]

It's the way you do config normally. BukkitConfiguration is just the worst way of doing config.

Do you have any thoughts of Ivan's use of AnnotationConfig in this PR?

The only thing I remark ist that the PR should be split.

1. module split
2. Config stuff
3. probably other stuff that are own pr's

3k LOC aren't well reviewable for me.

[MrIvanPlays]

Nono @yannicklamprecht he asks whether or not you approve to replace the homebrew config ( which currently sucks ) with AnnotationConfig.

[yannicklamprecht]

Nono @yannicklamprecht he asks whether or not you approve to replace the homebrew config ( which currently sucks ) with AnnotationConfig.

From the frontend standpoint I'm fine with that new config design.
That stuff that is currently implemented in this pr has a lot of overhead with the whole key thingy.
When stuff can be easily maintained I'll be fine.

[MrIvanPlays]

I think that the codemc deployment profile should not be done by me so it is gonna be left as a TODO.

Waiting for your 2 cents on the code :)

[lokka30]

Suggestion 1:
I think we should rename the bukkit module to core-bukkit

Suggestion 2:
At: bukkit\src\main\java\me\lokka30\treasury\plugin\bukkit\vendor\paper\PaperAsyncTabEnhancement.java Line 47, and
At: core\src\main\java\me\lokka30\treasury\plugin\core\command\subcommand\migrate\MigrateSubcommand.java Line 188:
We should instead iterate over the plugins which have registered as a Treasury service provider. Is that still possible?

Suggestion 3:
At: bukkit\src\main\java\me\lokka30\treasury\plugin\bukkit\vendor\paper\PaperEnhancements.java Line 10:
Wouldn't it be more reliable to check versions the way MicroLib does? (e.g. to check if the server is 1.12+, ensure Material.WHITE_CONCRETE exists. to check if the server is 1.15+, check if Material.HONEY_BLOCK exists)

Suggestion 4:
At: core\src\main\java\me\lokka30\treasury\plugin\core\command\subcommand\InfoSubcommand.java Line 54, 55:
We can probably replace this with "${project.version} and ${project.description}

Suggestion 5:
At: bukkit\src\main\java\me\lokka30\treasury\plugin\bukkit\Treasury.java
I think this class should be renamed to TreasuryBukkit

Suggestion 6:
At: core\src\main\java\me\lokka30\treasury\plugin\core\config\messaging\Messages.java Line 31:
Forgot a m in .yl (to make .yml)

[MrIvanPlays]

Suggestion 1: I think we should rename the bukkit module to core-bukkit

core implies that it doesn't have the full functionality of the plugin. In my opinion that's a no.

Suggestion 3: At: bukkit\src\main\java\me\lokka30\treasury\plugin\bukkit\vendor\paper\PaperEnhancements.java Line 10: Wouldn't it be more reliable to check versions the way MicroLib does? (e.g. to check if the server is 1.12+, ensure Material.WHITE_CONCRETE exists. to check if the server is 1.15+, check if Material.HONEY_BLOCK exists)

The best method is what it's currently using as it is guaranteed the craftbukkit package is gonna contain the version. No, materials are the worst way to check the versions because they're subject of a rapid change between releases.

Suggestion 4: At: core\src\main\java\me\lokka30\treasury\plugin\core\command\subcommand\InfoSubcommand.java Line 54, 55: We can probably replace this with "${project.version} and ${project.description}

Maven does not handle class filtering unless you have a maven plugin in the build and put the classes you need filtered in a special directory (a.ka src/main/java-templates or something, don't really remember). So no, we can't replace them. Either way you have the version replaced in the plugin.yml, and the current bukkit platform implementation code is getting them from there. Other platforms also have something like plugin.yml which is holding the version, so we're safe with the current implementation for now.

[lokka30]

Is this PR ready to be merged?

[lokka30]

@Jikoo, do you have any comments? I understand you are likely busy so don't worry about it if you can't.

[Jikoo]

I usually prefer using dependencyManagement and pluginManagement to properties for versioning in multi-module configurations, but it works fine and I'm okay with the current state. The restructure seems good, formatting is solid.

I apologize that I haven't looked at any of the configuration details, I spent most of my programming time recently prepping for 1.18.

[MrIvanPlays]

Is this PR ready to be merged?

If no one has anything else to say then yes.

[lokka30]

Merged - thank you very much Ivan and also to everyone who provided feedback on the PR!