MaxMaeder
contribution guidelines
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
- [x] Introduction
- [x] describe relevant Maven targets
- [x] Java IDE recommendation
- [x] specify Java formatt-er/ing used
- [ ] specify Java style guide used
- [ ] specify Java coding conventions not covered by or different from style guide
- [x] Java version target citation needed
- [x] Kotlin reasoning needed
- [ ] Everything Authenticator related
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
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.
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
[..] 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
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
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
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
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.
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.
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
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?
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
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?
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
the remaining points have been addressed
i also added a link to a short guide on how to setup build/run/debug for IntelliJ, this could maybe move into the wiki in the future with better formatting and pictures. (i did not find a existing one so i made my onw)
after reading through the whole thing again, i did not find anything else that stood out to me
anything related to JS / the authenticator is still missing but i dont know anything about that and it can always be added at a later date if a need arises.
so i went ahead and marked it as ready for review
stale. im also not sure if i would keep working on this. but feel free to reopen / use a base for future work.
one last note: clang-format does support java, if that works like it does for c++ it would fix the issues i had with the java formatters, but i have not tested it.