contribution guidelines

[StillGreen-san]

this resolves #158 by adding a CONTRIBUTING.md file that briefly explains how to contribute to the project

currently only a draft as it is unfinished

• Introduction
• describe relevant Maven targets
• Java IDE recommendation
• specify Java formatt-er/ing used
• specify Java style guide used
• specify Java coding conventions not covered by or different from style guide
• Java version target citation needed
• Kotlin reasoning needed
• Everything Authenticator related

[StillGreen-san]

through trial and error i settled on the publish package maven target to build the plugin. is that the right one, are any of the other targets relevant?

there is a .vscode folder in the repo, does this mean that VS Code is used by the maintainers?

the default formatter settings for VS Code and IntelliJ are fairly close to the current formatting (in the files i tested), are those used, maybe with slightly different settings?

edit: i meant the package target not publish

[rhld16]

Love the initiative, thanks for your help. Just laying in on a few points:

• Since we generally go for the ethos of max compatibility as well as simplicity, there’s unfortunately no easy way to get around building with java 8 if we’d like to support mc prior to 1.17. However, these older versions only make up less than -6% of our userbase so maybe a rethink is due on this.
• I personally use vscode when editing this plugin, but this is more to do with the fact java is not a language I frequent. Both @lrmtheboss and yourself seem to be better in tuned here, so I think your opinions would be more beneficial haha.

[lrmtheboss]

I use IntelliJ IDEA for Java projects, and all my settings are stored In my global settings.
In IntelliJ, I mainly use the maven target of clean and package.

• clean for clearing all previous build data
• package for building the jar

As for the Java formatting the main points I would include are

• 4 space indents
• replace all tabs with spaces

As for other rules I use most of the ones built into IntelliJ and the SonarLint plugin.
Simply as long as the code is similar to what is already in the repo, It'll be fine

Kotlin - I'd rather have normal Java instead

[StillGreen-san]

[..] and yourself seem to be better in tuned here [..]

for a bit of context, I technically only started using Java last month for this project. the last time I used it was like a decade ago and even then not that much.

I spend essentially all my time with C++

---

lets start with the Java version

whether or not to upgrade the used Java version is something that should probably be discussed separately

for this I was mostly interested in adding a bit of information on why exactly an old Java version is needed to support old Minecraft versions

but best I found was this comment about specific spigot versions

[StillGreen-san]

Java IDEs that come to mind are

• IntelliJ
• VSCode
• Eclipse

all of them can be used for free
personally I'd prefer to not require a specific IDE as long as they support the necessary features, but it might make some thing easier

---

for code formatting we could use the eclipse formatter xml format
this is seemingly supported by all 3 IDEs (mentioned above)

however, in testing there are still some differences between formatting with IntelliJ and VSCode when using a style defined with an eclipse xml see here
vscode also did not respect the tab character set in the style, manually setting it was required

whether or not the difference can be eliminated requires some more investigation

[lrmtheboss]

Another thing you could add is testing requirements for testing their changes

should be tested against the latest normally used Java version (17 currently) and on the latest release of Minecraft for each platform (when multi-platform support is added)
ideally, they should also have one world folder that is at least 5 gigs to make sure their changes are not broken with large uploads

java 17 and paper 1.20.4 is how I'm currently testing my changes

[StillGreen-san]

ok, before this auto formatting stuff is driving me insane I will settle on a good enough solution:

https://github.com/revelc/formatter-maven-plugin configured to automatically run on compile

my other attempts with other IDE plugins always resulted in differences between the IDEs that I could not work out

the maven plugin approach ensures 'consistent' formatting between IDEs and can also be used to validate formatting during CI runs
it only requires you to disable formatting in the IDE

one thing I noticed was that all the formatters are keeping custom line breaks, if it does not conflict with other format rules

also, newer version of the plugin require jdk versions above 8 see here

let me know if this approach is acceptable

[lrmtheboss]

If formatting is going to be a huge pain to do automatically then just a list of formatting rules should suffice. Worse case we may have to fix some of it ourselves before or after merging the PR.
As for newer versions requiring newer than JDK 8 that may be an issue until all the maintainers and developers can get together and discuss if we should upgrade to 11 or even 17.

[StillGreen-san]

well, it was only a pain to find something that works across IDEs (I hope that I just did something wrong, doesnt seem like the thing that should be complicated).
setting it up and using it is strait forward, its just that I would have preferred if it wasnt tied to the build system and instead could be easily run from the IDE direcly.

as for the jdk requirement, that is only for the developers, so that the formatter plugin can run.
the target java version (as specified in the pom) does not change.

[StillGreen-san]

for the style guide there are two options, writing a new one or using an existing one

from the ones I looked at I liked the (old?)twitter one here
as it has examples for its guidelines and a section for best practices

[StillGreen-san]

to continue with this i need a few decisions from you on the remaining points

• java formatt-er/ing
  • should the formatting be enforced via tooling (as described above)?
  • or not, and leave this a manual thing
• java style guide
  • use an existing one as a base (more complete)
  • or make new one (more focused)
• java version target citation
  • do you have a link explaining how/why old mc versions would not work?
  • is this because you dont want to require a version higher than what that mc veriosn requires?
  • or should this point be left as is?
• kotlin reasoning
  • can you elaborate on the kotlin reasoning?
  • or should this point be left as is?
• everything Authenticator related
  • should this be part of these guidelines?
  • if yes, do you have something to put there or should this remain as todo?

[lrmtheboss]

formatting

I'm fine with it being manual.

style guide

Either option is fine but we may want to take an existing one and modify it to fit our needs as that would be easier than making our own from scratch.

java version

Is this because you don't want to require a version higher than what that MC version requires?
This is a valid point, since we don't need any features from newer versions of Java there is no point in forcing people to run a newer version of Java especially since we currently support down to MC 1.8.

kotliin

With everything being so far in pure Java I'd rather keep it that way.
By allowing people to contribute in Kotlin it would add complexity to our code base and I prefer pure Java over any Kotlin

Authenticator

It should but I don't have anything to put there as I focus on the plugin

[StillGreen-san]

i used the (old?)twitter styleguide as a base (as mentionend earlier)
i already updated and changed a few things that stood out to me, but im not that familiar with this projects style

at this point you would need to read through the whole thing (styleguide.md) to take note of anything that needs to be changed, added or removed

some things that might warrent attention:

• #recommended-reading
  • cant say whether those are good or not
• #100-column-limit
  • a column limit seems not enforced in the codebase?
• #be-explicit-about-operator-precedence
  • the part about beeing really obvious seems a bit much to me
• #import-ordering
  • no consistent grouping in the codebase
  • but generally less groups than suggested here
• #writing-testable-code
  • currently no tests in this project
  • good opportunity to introduce a test setup to make later adoption easier (not necessarely as part of this pull)
  • #time-dependence references code not part of this repo
    • unless there is a good equevivalent available in the project, those can just be removed
• #clean-up-with-finally
  • should mention try-with-resource
• #todos
  • id say that issues should be preferred here
  • maybe todos only for non functional improvements?

i just realized, should i have left the comments as a review?

[lrmtheboss]

100-column limit

No column limit is enforced. For me, it depends on the context and surrounding code as to how long I'll let a line run

be-explicit-about-operator-precedence

When using more than 2-3 operators in a statement you should add clarifying parenthesis, so that it's clear what you mean. Especially when combining && and || in a single statement.

import-ordering

Import order doesn't matter, most IDEs will auto-add imports for you. Some will also reorder them like IntelliJ does when you run Optimize Imports. As long as they are present and contain no duplicate or unnecessary imports it should be fine.

clean-up-with-finally

Yes, it should.

Todos

For bugs or known edge cases, issues should be preferred
For notes like when feature x is added, remember to rework this method to support y where it is not a bug or edge case and can't be done until something else is done, I think it would be fine.
todo for future notes like once xyz updates redo this method to use new feature ABC from xyz should also be fine

time-dependence

no equivalent in this project that I can think of

writing-testable-code

Once we have a lot more platform-independent code (maybe after the platform-agnostic rewrite), that's when we should consider adding tests.
Currently, I think it's too tightly integrated with Bukkit's API to add proper tests

recommended-reading

Since they both are paid, I'd remove them.
If someone were to contribute, they should already have at least a basic understanding of Java and should be able to infer from the rest of the code how to do things.

I've read through the whole guide and I've got no other comments or notes at this time