-
-
Notifications
You must be signed in to change notification settings - Fork 57
Add contributing documentation #59
Add contributing documentation #59
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very great work! Just left some comments on readability. Perhaps a dev will swoop in and help with the more logical stuff :p
public static boolean saveEmptyScoreboardTeams = false; | ||
private static void saveEmptyScoreboardTeams() { | ||
// This is called automatically! | ||
// The name also doesn't matter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it not matter? We use it a lot so it may be clear to indicate that it should accurately reflect the config name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea how to respond to this. It's copypasta'd from CONTRIBUTING.md. Going to need someone else to let me know what to put here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might also mention using a new branch for your pull request. Something like this could possibly go under not using an org, but I'm not sure where else it would fit well as that isn't the best place. This also isn't in the existing CONTRIBUTING.md either. I'm also not sure how important this actually is or if it really warrants inclusion, but it seems to make things easier both for people not accidentally pushing other changes and when merging.
What line items need to be addressed here specifically? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just read over it and it all looks really good, just two minor things. Other than these it looks great to me, thank you for your work on this. However I'm not a dev working on paper, so it would be good for one of them read this over before merge.
Also given that much of this has changed with 1.17 and the process for this is still in development with the re-adding of the paper
tool, I would leave a note on the index page about that including things folks might run into, such as it changing to base
rather than upstream/upstream
.
|
||
- ``git`` (package ``git`` everywhere); | ||
- ``patch`` (often package ``patch``); | ||
- A Java 16 or later JDK (packages vary, use Google/DuckDuckGo/etc.). If you need one, you can find them on AdoptOpenJDK. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could link to upgrade to java 16 guide here.
# docker run -it -v "$(pwd)":/data --rm adoptopenjdk:8-jdk-hotspot bash | ||
Pulling image... | ||
|
||
root@abcdefg1234:/# javac -version | ||
javac 1.8.0_252 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# docker run -it -v "$(pwd)":/data --rm adoptopenjdk:8-jdk-hotspot bash | |
Pulling image... | |
root@abcdefg1234:/# javac -version | |
javac 1.8.0_252 | |
# docker run -it -v "$(pwd)":/data --rm adoptopenjdk:16-jdk-hotspot bash | |
Pulling image... | |
root@abcdefg1234:/# javac -version | |
javac 16.0.1 |
I don't think it makes much sense moving this out of the Paper repository just for the sake of it. Keeping both would be okay but means unnecessary redundancy, removing it from the repo is just inconvenient for single-monitor setups, especially for people that have never worked with patches before and now have to change windows constantly vs. just viewing it in IDE to read about the dozen steps you have to follow. |
PaperDocs is for documentation, this is documentation. But, if it shouldn't be put on the docs website then that's fine. Feel free to close, or I can keep working |
Thank you very much for your fantastic work on this! However, even though this site is for documentation, on further discussion, docs for developing paper itself will stay on Paper's repo while docs for using paper/its API can go here on this site, at least for the time being. We should have done this much sooner rather than having you do a bunch of work on this first/leaving reviews. Sorry about that. Thank you nonetheless. |
alright that's fine. i plan to work on the "user end" dev documentation when my school semester ends (#62). please let me know if the team does not want to have it on the RTD site in the next two or so weeks before i work on it :) |
I am attempting to work on PaperDocs in more than one area and will be submitting a few PRs. This PR is to get contributing documentation over to PaperDocs. There will likely need to be extra changes. If you see anything glaring please let me know and I will fix it before merge. I believe these ones are ready to go in.
This is essentially a carbon copy of the current CONTRIBUTING.md into RST and organized.
Let me know if anything else needs to be added/changed for this to go in. Can be iterated on.
More to come ^^