-
Notifications
You must be signed in to change notification settings - Fork 751
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
Docs: Rework of Contributing.md #2278
Conversation
To note: I am planning on adding a "quick guide" for tests. It will contain an overview of the most useful tests for logic, an overview of what is required to be tested by a core contributor, as well as a quick overview for webhost testing. |
Co-authored-by: black-sliver <[email protected]>
Separated the sentence specifically for web stuff as well as slight rephrasing of the first bullet point
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.
The other bullet points are also missing proper indentation.
This sounds like a great addition that a lot of people will find very useful. I'm suprised that we don't already have something like this. |
Co-authored-by: black-sliver <[email protected]>
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.
Quite good overall. One minor grammar nitpick.
docs/contributing.md
Outdated
* Turn on automated github actions in your fork to have github run all the unit tests after pushing. See example below: | ||
* **Follow styling guidelines.** | ||
Please take a look at the [code style documentation](/docs/style.md) | ||
to ensure uniformity and ease of communication. |
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.
Minor: This text is indented by two spaces when the following bullet point aren't. Note in either case, Markdown will combine the following lines with the above bullet point into one paragraph / list item. I assume this is intentional.
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.
yeah, this what i meant by
The other bullet points are also missing proper indentation.
The other listings should be fixed as well.
If you want to add Archipelago support for a new game, please take a look at the [adding games documentation](/docs/adding%20games.md), which details what is required | ||
to implement support for a game, as well as tips for how to get started. | ||
If you want to merge a new game into the main Archipelago repo, please make sure to read the responsibilities as a | ||
[world maintainer](/docs/world%20maintainer.md). |
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.
"please make sure to read the responsibilities as a world maintainer" is a little awkward. Consider one of:
- please make sure to read the responsibilities of a world maintainer
- please make sure to read your responsibilities as a world maintainer
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.
"please make sure to read the [responsibilities as a world maintainer documentation]"
I think that would be the most clear 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 will consider those changes
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.
anything?
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.
Some nitpicks, mostly
If you want to add Archipelago support for a new game, please take a look at the [adding games documentation](/docs/adding%20games.md), which details what is required | ||
to implement support for a game, as well as tips for how to get started. | ||
If you want to merge a new game into the main Archipelago repo, please make sure to read the responsibilities as a | ||
[world maintainer](/docs/world%20maintainer.md). |
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.
"please make sure to read the [responsibilities as a world maintainer documentation]"
I think that would be the most clear here.
Changed the order of two words Co-authored-by: Scipio Wright <[email protected]>
Clarified "this document" Co-authored-by: Scipio Wright <[email protected]>
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.
Lgtm. The world test link should probably change once we merge #2348
* Update contributing.md * Update docs/contributing.md Co-authored-by: black-sliver <[email protected]> * Update contributing.md Separated the sentence specifically for web stuff as well as slight rephrasing of the first bullet point * Update docs/contributing.md Co-authored-by: black-sliver <[email protected]> * Update docs/contributing.md Changed the order of two words Co-authored-by: Scipio Wright <[email protected]> * Update docs/contributing.md Clarified "this document" Co-authored-by: Scipio Wright <[email protected]> --------- Co-authored-by: black-sliver <[email protected]> Co-authored-by: Scipio Wright <[email protected]>
What is this fixing or adding?
Adding a lot of details to the contributing.md document to make it more beginner-friendly
How was this tested?
Feedback?
If this makes graphical changes, please attach screenshots.