# Guide to Reviewing a PR on Nova
Make sure to read the [Maintainer Guidelines](https://hackmd.io/5IbK5IaFR_u4B5ZpUvDVfw) first.
**First off, remember to stay cool and level-headed when reviewing a PR, even if you get triggered by something. Politeness and respectful communication goes a long way!**
An example workflow when it comes to reviewing PRs:
1. Check the PR title and body for errors/typos. Make sure there is nothing NSFW in the PR title and that NSFW content is hidden behind a \<details>\</details>.
2. Make sure that it has a changelog and proof of testing if needed (use your best judgement here/be reasonable). aka *you should not be asking someone for proof of testing for a spellcheck PR*.
3. Comb through each file in Files Changed, ticking 'Viewed' when you've finished with one. Ensure that you are using the "Start Review"/ "Add Review" buttons and not "Add single comment"
Refer to the [Common issues](##Common-issues-that-come-up-in-reviews) section below for an idea of what to be looking for in your reviews.
4. Check IconDiffBot/MapDiffBot outputs if it's a sprite or mapping PR to make sure there are no errors or regressions there.
* Note: Sometimes these glitch out and cannot be relied on though, so if the output looks off just double check that it's actually off before bringing it up in review.
5. Labels. Double check that appropriate labels have been applied.
* ESPECIALLY config_updates, sometimes the bot doesn't apply it properly. Add "GBP: No Update" for followup fixes to recent PRs. Add 'Good First PR' if it's their first PR with us.
5. Request changes when you're done, with any additional/final comments going there.
6. When the PR author makes the requested changes, you may either approve it if it is ready, or dismiss your review (in the case of it needing further review/a testmerge).
## Common issues that come up in reviews
Refer to this as a checklist of sorts until you know it by heart, which after a while of reviewing PRs it just becomes second nature after a point.
Look for:
#### Style mistakes
- [ ] **Spelling/grammar mistakes**. Yes, sometimes we are naught but glorified spellcheckers.
- [ ] **Incorrect Autodoc**. In order for VSC to display their comments on hover, they should be formatted as so:
```
/// This var shows you how to write an autodoc comment, using three ///'s
var/here_is_how
// I'm just a floating comment not tied to any proc or anything else, so I have two //'s
code
```
* For multiline comments [see here for more info](https://github.com/tgstation/tgstation/blob/master/.github/guides/AUTODOC.md#documenting-a-proc)
- [ ] **No single letter vars**. It's not allowed on TG anymore and isn't here either. Self-documenting vars are just better.
- [ ] **Lists without trailing commas/improperly formatted lists** e.g.:
#### Right:
```
materials = list (
/datum/material/iron = SHEET_MATERIAL_AMOUNT * 1.25,
/datum/material/glass = HALF_SHEET_MATERIAL_AMOUNT * 1.5,
/datum/material/silver = HALF_SHEET_MATERIAL_AMOUNT * 1.5,
)
```
#### Wrong:
```
materials = list (
/datum/material/iron = SHEET_MATERIAL_AMOUNT * 1.25,
/datum/material/glass = HALF_SHEET_MATERIAL_AMOUNT * 1.5,
/datum/material/silver = HALF_SHEET_MATERIAL_AMOUNT * 1.5
)
```
#### Also wrong:
```
materials = list (
/datum/material/iron = SHEET_MATERIAL_AMOUNT * 1.25,
/datum/material/glass = HALF_SHEET_MATERIAL_AMOUNT * 1.5,
/datum/material/silver = HALF_SHEET_MATERIAL_AMOUNT * 1.5,
)
```
- [ ] **Atoms that are named with a capital letter**. Byond will automatically count these as proper nouns, and refer to them as "<Item_name_here>" without an article in the examine text. Sometimes this is not desired, such as when you are referring to a brand name of some mass produced object (very common). Here is a hypothetical brand name item where you would want to force Byond to refer to it as 'a':
Bad
```
name = Shim-Sham-Jimmety-Jam-Clapper-Gapper
(shows up as "This is Shim-Sham-Jimmety-Jam-Clapper-Gapper")
```
Good
```
name = \improper Shim-Sham-Jimmety-Jam-Clapper-Gapper
(shows up as "This is a Shim-Sham-Jimmety-Jam-Clapper-Gapper")
```
Note: this only applies to atoms, not datums. And some atoms like circuit boards and some machines are fine to treat as a proper noun, so it's not a universal rule.
- [ ] On 'magic numbers': Encourage use of `#define`s for numeric and string values that are not self-evident.
* That does NOT mean every number has to be a define though. Don't be annoying with this! The goal is to make the code self-documenting + provide a single point of definition, and sometimes too many defines makes it harder to read in my opinion.
* A good rule of thumb is, if the same specific value is being used more than once then it would be a good thing to make into a define.
- [ ] Really long strings should be made multiline using the escape char. e.g.
```
desc = "My name is Mal and I have something to say. \
I forget what it was now. This line isn't even that long \
About 115 chars per line is a good stopping point as a rule of thumb.
```
- [ ] Look out for anything runs counter to the style guidelines in [STYLE.md](https://github.com/tgstation/tgstation/blob/master/.github/guides/STYLE.md)
#### NSFW Content
- [ ] **If there is NSFW content, ensure that NSFW prefs are being respected!**
* Whenever there is a NSFW item or interaction you need to check the appropriate prefs. People almost always forget this so be on the lookout.
#### Modularization errors - IMPORTANT!
- [ ] Check for undocced nonmodular edits
* You will often see someone trying to edit a TG file without tagging their changes. They need to either upstream the changes, mark the edit, or use a modular override. There is no one size fits all solution; use your best judgement here, or ask the other maints if you aren't sure what they should do.
- [ ] Whenever possible encourage modular overrides over nova edits.
* There is also no one size fits all solution here either so use your best judgement, especially when it comes to proc overrides.
- [ ] Nonmodular edits that are wrongly formatted
* Look out for nonmodular edit tags that are incorrectly formatted or placed. These should come in THREE forms (and yes make sure they are in this exact format, makes it easier to search for them if we have to!):
1) `// NOVA EDIT ADDITION`
2) `// NOVA EDIT REMOVAL`
3) `// NOVA EDIT CHANGE - ORIGINAL: the_full_line_of_code_here`
- Make sure that they are not accidentally adding nonmodular diffs outside of the edit tags. You will find people do that often. (e.g. see example from a Bubber port below, which also needs the tag changed to `// NOVA EDIT ADDITION START`)
#### Multiline examples:
##### Removals:
```
/* // NOVA EDIT REMOVAL START
code_here = commented out
*/ // NOVA EDIT REMOVAL END
```
##### Additions:
```
// NOVA EDIT ADDITION START
code_here = commented out
// NOVA EDIT ADDITION END
```
- [ ] Avoid multiline `// NOVA EDIT CHANGE`'s.
* For example, if it's something like indenting a section of code, surround the non-indented original code in a removal block and just have an addition block with the indented code. It's just less confusing this way when sorting conflicts. Example below.
```
/* // NOVA EDIT REMOVAL START - ORIGINAL
something = something_else
*/ // NOVA EDIT REMOVAL END
// NOVA EDIT ADDITION START
if(some_condition)
something = something_else
// NOVA EDIT ADDITION END
```
- For more info on modularization, and examples of how to do tgui edits refer to [the modularization guide](https://github.com/NovaSector/NovaSector/tree/master/modular_nova).
#### Be mindful of Proc overhead
- [ ] Check that a proc is not being overridden multiple times, especially if it a hot proc, and have them condense them into one override.
* Proc overhead is an actual concern in DM which has no inline functions or optimizations on that front. We should always try to cut back on the amount of overrides/limit it to one per item. For example, I've seen people try to override procs like `Login()` or `Topic()` multiple times across several modules, where there should really only be at most one singular override in `master_files`. It also makes it less confusing to track the control flow when there is one override at most per thing.
- [ ] Scrutinize new variables being added near the base of the obj tree to things like `atom/obj/item` or `/mob/living`.
* A lot of the times it is not necessary to do so. e.g. instead of adding a `var/cooldown_for_thing` on `/mob/living` look at what they are trying to do and try to find a better solution. (e.g. maybe a status effect, which come with built in timers, or perhaps an element or component).
- [ ] Scrutinize things that process() heavily
* Be extra careful of things that `process()`. We want to reduce the amount of things that are doing this where possible. Most of the time it's not actually necessary and whatever they are doing can just be accomplished using signals or some other method instead.
- [ ] Make sure that proc overrides include all the args from the parent proc.
* Especially true for process; make sure that their `process()` implementation includes the `seconds_per_tick` arg and that they are using it in their calculations to account for differences in tick configurations. `seconds_per_tick` is `2` by default for process() so divide by 2 to get the conversion. e.g.
```
/obj/thing/process(seconds_per_tick)
some_value += 1
```
becomes
```
/obj/thing/process(seconds_per_tick)
some_value += (0.5 * seconds_per_tick)
```
* This is specifically for `process()`. Do also note that other procs with a `seconds_per_tick` arg might tick at different rates (I believe Life() is every 4 seconds, for example). Same logic applies there, just account for the different timings.
#### Look out for DM-specific syntax oddities
- [ ] **Make sure that there are conditionals are wrapped appropriately in () for order-of-operations**
* as this can lead to errors if not done for certain things; for example checking if something isn't in a list:
```
if(!something in things)
```
Incorrect, will not work how you think it will ^
```
if(!(something in things))
```
Correct ^
* It's good practice in general to be overcautious with parenthesis, because DM is very weird. Do _not_ trust DM.
- [ ] Avoid creation of abstract types, aka "Debug" items wherever possible
* e.g. someone makes
`/obj/item/clothing/sweater/ugly/grandma`
`/obj/item/clothing/sweater/ugly/grandma/green`
but `/obj/item/clothing/sweater/ugly` itself is not defined. This creates an unnecessary debug item that can potentially be spawned and just takes up space.
* Instead of doing this, just have the basetype be the actually-in-use item. So the grandma sweater can be `/obj/item/clothing/sweater/ugly`. There are some instances where you would want to be using an abstract type for organizational purposes for `/datum`s but it's almost never necessary with items, and I see it often cause issues. Not to mention it just needlessly pollutes the obj tree.
#### Item repaths
- [ ] If there are item path changes, they should be providing an UpdatePaths script.
#### Sound files/Icons
- [ ] Sound files should be loudness-normalized and should come with an attributions.txt that credits the source.
* Make sure they are not using copyrighted assets (icons or sound) without permission.
- [ ] Make sure that there are no regressions or invalid icon_states by checking Icondiffbot.
#### Mapping
- [ ] Varedits are generally frowned upon, they should create mapping helpers and items instead.
- [ ] Watch out for nonmodular edits to TG maps.
* We have an automapper system that can be used to place modular edits onto TG maps which should be used instead, as well as an area spawn system that can be used to modularly place things semi-randomly in a room.
- [ ] Check Mapdiffbot to get an overall idea of the changes.
- [ ] Make sure that no overpowered/debug items are being hidden in the map somewhere (yes we've had this happen)!
#### Loadout Guidelines
Loadout items should:
- [ ] Have minimal mechanical value
- [ ] Be accessed with relative ease at the start of the round, without notable requirement of ID access, resources, effort or time. NOTE: Ease of acquisition does not surpass previous rules
Sign in with Wallet
Connect another wallet