Things to look for in code review - elanthia-online/dr-scripts GitHub Wiki
Code review legend
If needed, include the legend for code review comments:
:bulb: This comment is an idea or recommendation. The PR can be merged without addressing it.
:question: This comment is a question that needs to be answered before I can properly review the PR. The PR cannot be merged until the question is answered.
:x: This comment is a concern that I would like addressed before the PR is merged. Please make a code change addressing the comment, or have a conversation with me to assuage my concerns.
get_settings and script args
It's not as easy as calling get_settings
if the script can take args which represent other YAML files to load or settings to overwrite. For example, see https://github.com/rpherbig/dr-scripts/pull/2239/files#r157391191
multiple functions with the same name (in common*)
See https://github.com/rpherbig/dr-scripts/pull/2231#issuecomment-352250005
functions in common* being added or renamed
See https://github.com/rpherbig/dr-scripts/pull/2192#issue-280696904
missing module reference in common* functions
See https://github.com/rpherbig/dr-scripts/pull/2231/commits/74a463e49beec670e4aa4b15af607b3366d4fd9f
missing reference in custom_require
See https://github.com/rpherbig/dr-scripts/pull/2292/files#r160014027
avoid use of fput (in favor of bput) whenever possible
TODO: Find example
setupalias additions need double escaping
See https://github.com/rpherbig/dr-scripts/pull/2013
subkeys in base.yaml are not inherited if the key already exists and need safeguards
necromancer_healing:
wound_level_threshold: 1 # 1 - 8
was added to base.yaml. but most necromancers already had a necromancer_healing: defined and did not pick up the new subkey causing errors. See https://github.com/rpherbig/dr-scripts/pull/1998
base.yaml
All settings should have a default value defined in Self explanatory maybe?
base-empty.yaml
If a new setting is created that is an array or hash, it needs a corresponding entry in See https://github.com/rpherbig/dr-scripts/pull/2311#discussion_r160055168
If a script has Flags.add, it should add Flags.delete in before_dying.
Only reason not to would be if you depend on those flags in multiple scripts, which should only happen in common-* files.