Repository navigation
Contributing pull requests
How a pull request to upstream is shaped, reviewed and merged, measured from the pull requests merged into game_rework since 2024. Taking one of ours from a branch to an open pull request is Taking a change upstream; why upstream works this way is on Contributing.
| Change | Target |
|---|---|
| Most changes |
NexusForever/NexusForever, branch game_rework
|
| Combat, AI and instances |
expedition-evil-from-the-ether (#567) |
| A standalone tool |
NexusForever/NexusForever.Tools (#564) |
| Something upstream is already reworking | Ask on Discord first. Outside fixes there tend to be closed and redone (#574) |
Which upstream branch holds what is on Upstream branches.
- One concern per pull request. Reviewers ask for unrelated renames and namespace moves to go in their own (#501).
- Messages and behaviour apart. A message family lands as " messages": models, opcodes and enums, and no handlers. The behaviour that uses them comes in a later pull request.
- Small. Outside the message families, most pull requests change fewer than 100 lines.
A design question goes to the project Discord before it goes in a pull request. Threads then report the decision ("as discussed on Discord").
Nothing runs on a pull request: there's no CI. Build it in Debug, which checks the game table layouts at start-up, and run it in game. The maintainer tests by playing too, and says so when they couldn't.
The maintainer's own commits are past-tense sentences with a full stop, with any further points as - lines straight underneath:
Moved Who search functionality to Character server.
- Added a new query database to reduce the need to query the character database during who and player info queries.
Contributors' commit messages vary, and nobody asks them to change. But a squash copies every commit headline into the landed commit's body, so keep the headlines clean or keep to one commit.
If the change alters a schema, include its migration. Pull requests have landed without one, with the maintainer generating it on the branch, but a correct one saves the round trip. How to generate one is in Adding a player feature.
Leave Source/Directory.Packages.props and package references to the maintainer. Before reaching for a library, check whether the server already has what you need: a pull request adding pet renaming was pointed at the built-in TextFilterManager for checking names (#454).
Most pull requests land as a squash under GitHub's default subject, <title> (#N), so the title becomes the line in game_rework's history. Write it the way it should read there.
| Form | Example | Use |
|---|---|---|
| A plain phrase | "Housing messages" | Most since 2025 |
Area: summary |
"Network: close the socket even when shutdown fails" | Also common |
[Area/Sub] summary |
An older form; avoid |
A few short paragraphs or a bulleted list: what it does, what it builds on ("Builds on #587"), and what was tested and what wasn't. There's no template and headings aren't expected. A message family's body is often just a list of what was added, renamed or moved.
A claim about how the client behaves needs something a reviewer can check: the client's own UI script by name, a game table and what it holds, or a capture. Reviewers challenge logic that doesn't look like the client's ("I would be surprised if this is the logic the client used", #583), and the answer that settled it named the script. State what the client does and where to see it, rather than pasting its code.
- Leave "Allow edits by maintainers" ticked. The maintainer pushes fixes to contributors' branches.
-
Not ready yet: a
[WIP]prefix on the title is the usual signal. The maintainer drops it when it's ready to merge. - No labels, and closing links to issues are rare.
The maintainer is the only one who merges and the only formal reviewer. Regular contributors comment, and their comments carry weight.
| Area | What reviewers ask for | Example |
|---|---|---|
| Client message properties | private set |
"To keep things consistent, client message properties should be private set." #491 |
| Bit widths |
ReadEnum and WriteEnum, with the width |
#493 |
| Collections | = [] |
#493 |
| Spelling | British | "British English for consistency." #525 |
| Namespaces | The folder path | "Seems namespace doesn't match the file location." #495 |
| Names | Area first, consistent across a family | "To keep things consistent, would calling this ServerPetClearFlair be better?" #516 |
| Names from the client | The client's own, where it has one | "where possible it should match the client if we have it" #496 |
| Layering and reuse | Existing extensions and managers over new ones |
ToNetwork() #504, TextFilterManager #454
|
| Behaviour | Nothing existing broken, failures handled | "This breaks Player::SendSystemMessage" #523, "What would happen if the teleport failed?" #581 |
| Client fidelity | Logic the client would use, with evidence | #583 |
| Scope | Unrelated changes in their own pull request | #501 |
Style misses often get fixed by the maintainer on your branch, and so do missing migrations. Expect renames too, so the expedition branch can merge back cleanly. Questions about behaviour, fidelity and layering wait for you.
Upstream doesn't mind whether a branch catches up by merging game_rework in or by rebasing, and the maintainer does both on contributors' branches. Our own rule is stricter (Contributing).
| Kind | What happens | Example |
|---|---|---|
| Tests | Removed before the merge. Whether upstream wants tests is an open question, not a settled one | #571 |
| Docker or compose files | Closed. Aspire is the supported way to run the stack | #560 |
| A pull request with no changes | Closed, with a request to resubmit | #555 |
| Work already done on another branch | Closed and pointed there | #567 |
| A fix to something being reworked | Closed and redone the maintainer's way | #574 |
| Pull request | Lands as |
|---|---|
| Several commits | A squash: <title> (#N), with each commit's headline as a * line in the body |
| One commit | Usually a merge commit, "Merge pull request #N from <user>/<branch>" |
Authorship stays with the contributor. When the maintainer has pushed to the branch, GitHub adds them as a co-author.
Merges come in bursts. Half of the pull requests merged since 2024 waited under two weeks; some waited more than a year.
When the maintainer reimplements a contributor's pull request in the current architecture, the original is closed, and the new commit names the pull request it's based on.
Upstream takes bugs as GitHub issues with [Bug] in the title, and only for features that exist: something not built yet isn't a bug. The template, from the bug submission page:
| Field | What goes in it |
|---|---|
| Title | A short summary with a category: UI, Faction, World, Housing |
| Date | When you saw it |
| Description | Everything relevant |
| Occurrence | Once, sometimes or always |
| OS | Your operating system |
| Branch/Commit | What you ran. The template still says master or develop; give game_rework and the commit |
| STR | Steps to reproduce |
| Observed | What happened |
| Expected | What should have happened |
Screenshots are welcome.
| Piece | Path |
|---|---|
| Package versions | Source/Directory.Packages.props |
| Migrations | Source/NexusForever.Database.<Name>/Migrations/ |
| Line-ending rule | .gitattributes |
| Upstream's written rules | Contributing, bug submission |