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

Improve variable naming in EditorInspector::update_tree #97181

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

npinsker
Copy link

@npinsker npinsker commented Sep 19, 2024

Rename some variables to be more descriptive and move some that were far away from where they're used. No behavior change.

editor/editor_inspector.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
editor/editor_inspector.cpp Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

AThousandShips commented Sep 19, 2024

Thank you for your contribution, we generally avoid these kinds of surface level changes without good reason, and many of these changes actively violate the comment style guidelines (including removing just the period from a number of comments)

The general philosophy for code comments is that they should help explain the code itself, and clarify things that might not be easy to get from the code, but code should be self explanatory, we don't use comments as documentation

@AThousandShips AThousandShips dismissed their stale review September 19, 2024 11:37

Style concerns resolved

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

As commented above I'd say that this PR shouldn't mix renaming variables or moving code with surface level changes, so comment changes should be separate from reordering or renaming variables, as changes to the code itself, even while it shouldn't affect behavior is something we should make sure we track well

Further strictly changing comments is very different and would be discussed far more so it'd hold back any other style changes, and might be rejected entirely on their own, we can also hide the comment changes from the history to make this PR less bad for the git history

@npinsker
Copy link
Author

Thank you for the quick comments! Will remove the code comments

@npinsker npinsker changed the title comments + context for update_tree in editor_inspector.cpp improve variable naming in update_tree method Sep 19, 2024
@npinsker
Copy link
Author

Thanks for the feedback -- I hope the PR's mildly helpful but regardless it is definitely extremely surface-level :) No worries if it isn't how you do things, feel free to reject

@AThousandShips AThousandShips changed the title improve variable naming in update_tree method Improve variable naming in EditorInspector::update_tree Sep 19, 2024
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

I actually am not against this, assuming the terminology is correct. The code being shifted brings it closer to where it's actually used, which makes the intent clearer. It does not functionally break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants