Fix/docs #812
Fix/docs #812
Conversation
ImMorpheus
added
the
needs review
There was a problem hiding this comment.
Thanks for the clean-up. Someone else should review the code snippets. The rest seems good, especially having https everywhere. I want to comment on the "things left out":
- Move lantern link to their website
- it's also nice to know the project is on GitHub, we could have both.
- Update testplugins guidelines (Track Enhancements of Debugging Documentation #793)
- may justify a separate PR
- WIP prefix for PR should be discouraged
- please do it.
- custom manipulators using code from broken myhomes testplugin.
- Agreed, that means either the testplugin gets fixed, a new code example is written, or we make it disappear (for now). It may justify a separate issue and PR.
There was a problem hiding this comment.
The Minecraft version doesn't need to prevent merging, but the sentence with the extra article needs correcting while it is getting updated anyway.
Newb ALERT: I inadvertently clicked on the first "View Changes" button instead of the last one. I hope this doesn't cause problems.
| | *8.0.0* | TBA | TBA | * *SpongeForge (1.13.x)* | | ||
| | | | | * *SpongeVanilla (1.13.x)* | | ||
| | *8.0.0* | TBA | TBA | * *SpongeForge* | | ||
| | | | | * *SpongeVanilla* | | ||
| +-------------+--------------+----------------+-------------------------------------------+ |
There was a problem hiding this comment.
Is leaving the Minecraft version off intentional? Gabizou said to change it to 1.14, but I can agree with leaving it off for now given the amount of change and uncertainty for which Minecraft version will have fully-functional implementations.
There was a problem hiding this comment.
Six of one, half a dozen of the other... Leaving the MC version off an as-yet unreleased API is probably OK, as it will definitely have one by release.
There was a problem hiding this comment.
Is leaving the Minecraft version off intentional?
Yeah, this is the docs for api 7. No need to worry about future versions of the api.
| can confirm the issue and know that you're working on fixing it. You should also create a WIP (work in process) pull | ||
| request prefixed with ``[WIP]`` early so we can already start reviewing them. | ||
| can confirm the issue and know that you're working on fixing it. You should also create a draft pull | ||
| request or comment with ``~wip`` so we can already start reviewing them. |
There was a problem hiding this comment.
I am not certain of what is meant by
or comment with
~wip
Should the comment be made in the issue or the pull request?
Perhaps one of the following two sentences would be clearer:
You should also create a draft pull request with a ~wip comment so we ...
OR
You should also create a draft pull request or comment in the issue with ~wip so we ...
There was a problem hiding this comment.
In theory WIP status can be automatically set in GitHub now. More detail is probably required.
There was a problem hiding this comment.
I see. I found the following URLs. Maybe a link in a Tip block to one of them for more information will suffice.
https://github.blog/2019-02-14-introducing-draft-pull-requests/
https://help.github.com/en/articles/about-pull-requests#draft-pull-requests
There was a problem hiding this comment.
I was referring to limbo.
https://github.com/SpongePowered/limbo-config/wiki/Pull-Request-Commands
Should also fix #640
Things I left out:
Update testplugins guidelines (Track Enhancements of Debugging Documentation #793)
the section about custom manipulators is using code from the myhomes testplugin. That plugin is broken. It should not be used as an example in its current state.
SpongeDocs/source/plugin/event/custom.rst
Lines 40 to 44 in 23b8ce8
is not wrong, but it's not really a good practice either. You should create an EvenContextKey and move the
Mutationto the context.