Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code style fixes and guidelines #2019

Merged
merged 2 commits into from
Nov 29, 2023
Merged

Conversation

fenhl
Copy link
Collaborator

@fenhl fenhl commented Jun 24, 2023

This PR includes various code style fixes that have been accumulating on my fork for a while. Most things are minor, like fixing typos in variable names, adjusting whitespace, or removing unnecessary parentheses, but there's also a couple instances where code duplication is removed, and replacing lists with tuples in the Python code should be a (very small) performance improvement.

I've also updated CI.py to include a few more checks and files. Where adding automated checks wasn't feasible, I've tried to formalize the changes I've made and some other existing conventions into a new document Notes/code-style.md. Here's my rationale for some of the guidelines that might be more controversial:

  • Commented-out code: Dead code adds bloat to the codebase without accomplishing anything. Commented-out code is always dead, so it must serve as documentation to be useful.
  • Parentheses around and inside or: This formalizes existing practice in the codebase, especially in the logic files.
  • Uppercase hex digits: This is the opposite of what I'm used to, since lowercase letters tend to be more distinct from digits. But uppercase is what's used in most of the codebase, so I'm adding this guideline so people like me don't accidentally introduce inconsistent style.
  • Pointer variables: This reflects the fact that in int *a, b, b is of type int, not int *. Even though we currently don't use multiple variable declarations like this (at least as far as I'm aware), not being aware of this quirk of the language can lead to subtle bugs, so I think it's best not to obscure it with the formatting. Overruled by the dev team.
  • Trailing comma: This improves readability of diffs where elements are added or removed from such a list.
  • Tuples instead of lists: This is for performance reasons. If a tuple consists of only literals, the Python parser will in turn treat it as a literal, which means that it will only ever be constructed once per program execution, while an equivalent list would be constructed each time that line of code runs.

@fenhl
Copy link
Collaborator Author

fenhl commented Jun 24, 2023

Removing the code duplication from Hints.hint_dist_keys revealed a bug where the important_check hint type was missing from that set. Fixing it caused unit tests to fail because some plandos are missing that hint type from their custom hint distro and important_check wasn't one of the hint types that were filled in with default values for backwards compatibility. I've decided to add this default for all hint types rather than special-casing just the newer ones. This will allow hint distros to be less verbose by completely omitting all unused hint types.

@fenhl fenhl added Type: Bug Something isn't working Component: Hints related to how we help the player Component: Misc A catch-all label Component: Documentation Affects user-facing help messages or public API docs labels Jun 24, 2023
ASM/c/actor.c Outdated Show resolved Hide resolved
@cjohnson57 cjohnson57 added the Status: Needs Review Someone should be looking at it label Jun 24, 2023
@fenhl fenhl requested a review from cjohnson57 July 30, 2023 21:39
fenhl added a commit to fenhl/OoT-Randomizer that referenced this pull request Sep 1, 2023
@flagrama
Copy link

flagrama commented Nov 1, 2023

What about a .clang-tidy config file so I can just feed that to my editor and have it ensure the correct style? (and that it won't undo style changes) I don't know how much it can do, but something like the pointer declaration syntax is probably something it handles.

@cjohnson57
Copy link
Collaborator

What about a .clang-tidy config file so I can just feed that to my editor and have it ensure the correct style? (and that it won't undo style changes) I don't know how much it can do, but something like the pointer declaration syntax is probably something it handles.

That would be a good thing to have, but I won't require it as part of this initial PR

Copy link
Collaborator

@cjohnson57 cjohnson57 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good to merge this as soon as we have any 8.0 bugs taken care of.

@cjohnson57 cjohnson57 merged commit d203790 into OoTRandomizer:Dev Nov 29, 2023
3 checks passed
@cjohnson57 cjohnson57 mentioned this pull request Nov 29, 2023
@fenhl fenhl deleted the code-style branch November 30, 2023 01:51
@fenhl fenhl added this to the 8.1 milestone Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Documentation Affects user-facing help messages or public API docs Component: Hints related to how we help the player Component: Misc A catch-all label Status: Needs Review Someone should be looking at it Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants