Things to look for in code review

Code review legend

If needed, include the legend for code review comments:

💡 This comment is an idea or recommendation. The PR can be merged without addressing it.

❓ 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.

❌ 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

All settings should have a default value defined in base.yaml

Self explanatory maybe?

If a new setting is created that is an array or hash, it needs a corresponding entry in base-empty.yaml

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.