Doc: review guidelines

[black-sliver]

What is this adding, removing or changing?

Defines worlds as dependencies, clarifies things in contribution guidelines and adds review guidelines.

Why is the change being made?

This is an attempt at making PR turnover faster:

For worlds, the world maintainer is in charge.

For core PRs, the review process is defined to make it less likely for PRs to get stuck in limbo.

Related discussion was on Discord.

How should this be tested?

By reading it for things that differ from what expectations are/were (when we talked on Discord) as well as for grammatical or spelling errors.

[Seatori]

Before anything else, I want to say that these changes and additions are greatly appreciated. Knowing what to look for with testing, encouraging developers to explain where things should be tested, and having clear documentation on how to approach creating PRs are all things that I would have loved to have in the past and will be wonderful to have going forward.

[Ixrec]

Probably agree. For what it's worth, the pattern I'm used to at my day job is a PR template question like "How was this tested?", on the presumption that no PR should be reviewed and merged before the appropriate amount of testing has been done (and "the appropriate amount is none" requires justification), and separately a PR template question like "Does this require any special post-merge/post-release steps?".

That last section was usually blank or even deleted by the PR author, since its purpose is to highlight the rare cases where we need to remember to do something after clicking merge. Once in a while that section included manual tests which were only feasible on beta/prod for whatever reason. In other words, any post-merge testing would be highlighted as a super weird extra step in a section most PRs would leave blank.

I'm not sure if that's appropriate here since most AP PRs are not automagically released to a beta environment right after being merged, but that's what I'm used to.

[black-sliver]

The idea is that you describe how to test it and then you test it as part of the self-review and anyone else can reproduce your results by following the description, but I understand that this may not be obvious.

Should I just revert to what we had before?

[duckboycool]

I think adding an explicit ask for if there's more testing needed could be good, but maybe it should just be done be changing to a single "How was this/should this be tested?" I think most people have currently just put this information under the existing part when applicable.

[black-sliver]

"/" or " and " ?

[beauxq]

These things (parentheses or "/" or "and") are what I was talking about with this:

I thought about maybe putting something in parentheses after "How was this tested?"
but it's hard to think of something that doesn't sound like "testing is optional"

When people read something like
"How was this tested and how should it be tested?"
they could think:
It looks like they don't expect "How was this tested?" and "How should this be tested?" to have the same answer,
which means they don't expect me to do the all testing that should be done.

If we just ask "How was this tested?"
It will be a common and reasonable assumption that they should do the testing that they think should be done,
so the answer to the question "How should this be tested?" should be contained inside the answer to "How was this tested?"

So "How was this tested?" will usually get the information we're looking for.
(And in the other cases when it doesn't, I can't think of any general wording that would make it more likely.)

[beauxq]

Maybe something like:

## How was this tested?
(Give information so that reviewers can reproduce your testing.)

?

[black-sliver]

I pushed this now.

The more important part than "reproduce" would be seeing which edge cases may have been missed, but I think for the unknowing PR author, it does not add value to go more into detail there.

[Ixrec]

The CODEOWNERS sentence made me realize... is this only about core-verified worlds, or does some/all of this apply to custom worlds too? (I'm genuinely unsure which is intended)

[black-sliver]

Well, the document is about contributing to this repo. Not sure how to include external worlds into this, but there could be a sentence to avoid confusion?

[black-sliver]

I updated the sentence about CODEOWNERS.
I think the section about how to contribute to an existing world is the same for core and non-core, the files are just in a different place.

[agilbert1412]

I'm a big supporter of these changes, and it's very well-written