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

Unify member variable naming #9522

Closed
wants to merge 23 commits into from
Closed

Conversation

ninetailedtori
Copy link
Contributor

@ninetailedtori ninetailedtori commented Mar 3, 2025

Describe your PR, what does it fix/add?

#9061 - Unification of code styling guidelines w.r.t. our contribution rulesets. This is a major refactoring change, and as such will likely be quite broken until everything is fixed. There were also a ton of typos and grammatical fixes I included.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

  • There are a LOT of variable name changes, which are still happening in this PR. There WILL be breakages.
  • clang-format and clang-tidy, I'd like to update them to a newer version in our GH workflows as well, but don't know too much about it. I threw a few errors with formatting on my own fork, regarding things like ReflowComments: 'Indent' etc.
  • There are some additions of files. These are just forward declarations in header and creation of source files. This is to reduce compile times and stay consistent with style guidelines.

Is it ready for merging, or does it need work?

  • Complete?
  • Tested?
  • Supported testing from others?
  • Patches required?
  • Patches done?
  • Merged?

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

can you remove all changes that are not in .hpp or .cpp files? (well or .clang-tidy)

@ninetailedtori
Copy link
Contributor Author

can you remove all changes that are not in .hpp or .cpp files? (well or .clang-tidy)

Ok, this problem will be dealt with at a later stage. Unable to shift them to a new branch right now. I'm busy going through all my changes and typo-proofing them, as the editor was a bit wacky to what I wanted. I could also just...branch this entire changes off, and then reset this PR to upstream head, then redo the changes again if you want?

@vaxerski
Copy link
Member

vaxerski commented Mar 3, 2025

up to you

@@ -62,4 +62,4 @@ BraceWrapping:

AlignConsecutiveDeclarations: AcrossEmptyLines

NamespaceIndentation: All
NamespaceIndentation: All

Choose a reason for hiding this comment

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

Why would you remove the newline at the end of file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for letting me know. I'll handle it in a bit. Or you can if you like help and PR a bunch of checks to my fork, but I'd rather handle it in batch.

Choose a reason for hiding this comment

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

Thanks for answering respectfully, I didn't realise this was still heavily a work-in-progress, that's my bad.

Good luck making your changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the well wishes and understanding. 😄

@ninetailedtori
Copy link
Contributor Author

ninetailedtori commented Mar 4, 2025

Chat... this literally says it's nowhere near complete. Please stop commenting on typos and weird newline changes. I didn't do them all, many were caused by vsc and clang-tidy, and I'm going through them manually, as I have been. Please leave this thread alone until I send a comment here stating it's ready for checking. Thank you for your understanding! 🙂

@vaxerski
Copy link
Member

vaxerski commented Mar 5, 2025

maybe mark as draft then?

@ninetailedtori
Copy link
Contributor Author

maybe mark as draft then?

Err I actually don't know how to do that...should I just delete the PR and redo it as a draft?

@ninetailedtori
Copy link
Contributor Author

Redoing this one as a draft, moving thread.

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