Skip to content
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

contribution guidelines #159

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

StillGreen-san
Copy link
Contributor

@StillGreen-san StillGreen-san commented Apr 7, 2024

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
Copy link
Contributor Author

StillGreen-san commented Apr 7, 2024

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
Copy link
Collaborator

rhld16 commented Apr 7, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

[..] 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
Copy link
Contributor Author

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
Copy link
Collaborator

lrmtheboss commented Apr 13, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contribution Guidelines
3 participants