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

Add and run pre-commit config #172

Merged
merged 7 commits into from
Jan 20, 2025
Merged

Conversation

WSSDude
Copy link
Contributor

@WSSDude WSSDude commented Jan 18, 2025

Requires Python 3.x with pip installed. Should be documented if someone wants to use it, as pre-commit hooks in general have to be set up manually after check-out, no way to automate this even if it did not use Python.

This should be executed in terminal opened inside the cloned repo folder to install and start using it (is one time, auto-updated after):
pip install pre-commit && pre-commit install

NOTE: Review by commit

  • first is just the config
  • second is output from running the config (probably no need to check every single thing, it seems all are trailing white-space apart from clang-format getting applied on few files)
  • fourth is adding the info to CONTRIBUTING.md on how to set up pre-commit
  • fifth is fixing reflection so new dumps do not have the trailing whitespace issue
  • sixth is refresh of natives after all of the changes above with RTTIDumper
  • seventh excludes vendor folder from all pre-commit hook stages and reverts changes that were done by second commit in vendor

wopss
wopss previously approved these changes Jan 18, 2025
Copy link
Owner

@wopss wopss left a comment

Choose a reason for hiding this comment

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

Thank you! Is it possible to disable the hooks for vendor directory?

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
@WSSDude WSSDude force-pushed the feature/pre-commit-hook branch from 0745453 to 2fbfdb0 Compare January 18, 2025 21:01
@WSSDude WSSDude marked this pull request as draft January 18, 2025 21:05
@WSSDude
Copy link
Contributor Author

WSSDude commented Jan 18, 2025

Let me try to run the RTTIDumper quickly to check the last commit and I believe this should be clean 👍

@WSSDude
Copy link
Contributor Author

WSSDude commented Jan 18, 2025

@wopss Seems the generation is clean now 👍
But the RTTIDumper detected one changed variable name in natives... Included the change in separate commit.

Also a bit unsure after I saw Natives.cpp if it should not be including *-inl.hpp files instead tbh 🤔 But left that one as-is.

@WSSDude WSSDude requested a review from wopss January 18, 2025 21:46
@WSSDude WSSDude marked this pull request as ready for review January 18, 2025 21:49
@WSSDude
Copy link
Contributor Author

WSSDude commented Jan 18, 2025

Thank you! Is it possible to disable the hooks for vendor directory?

Oh and to this - excluded, sorry... Fell out of mind when it was not a review comment 😄

Fixed in latest commit

CONTRIBUTING.md Outdated Show resolved Hide resolved
@WSSDude WSSDude force-pushed the feature/pre-commit-hook branch from 699d145 to 11d085e Compare January 19, 2025 11:48
@wopss
Copy link
Owner

wopss commented Jan 19, 2025

@wopss Seems the generation is clean now 👍 But the RTTIDumper detected one changed variable name in natives... Included the change in separate commit.

Also a bit unsure after I saw Natives.cpp if it should not be including *-inl.hpp files instead tbh 🤔 But left that one as-is.

We don't have -inl.hpp files which are generated, That's why it doesn't include them.

Copy link
Owner

@wopss wopss left a comment

Choose a reason for hiding this comment

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

Thank you!

CONTRIBUTING.md Show resolved Hide resolved
@wopss wopss requested a review from psiberx January 19, 2025 16:10
@wopss wopss merged commit f292dbb into wopss:master Jan 20, 2025
9 checks passed
@WSSDude WSSDude deleted the feature/pre-commit-hook branch January 20, 2025 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants