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

Configure pre-commit hooks and GitHub Actions #95

Merged
merged 12 commits into from
May 5, 2024
Merged

Conversation

azriel1rf
Copy link
Collaborator

@azriel1rf azriel1rf commented May 2, 2024

This Pull Request introduces several changes to improve the project's code quality assurance processes and refactor the CI workflows.

Python Formatting and Linting

  • Removed Black and isort and added Ruff for Python linting/formatting
  • Configured Ruff, mypy in pyproject.toml

C++ Formatting

Pre-commit Hooks

  • Set up pre-commit hooks via .pre-commit-config.yaml
    • General file checks (trailing whitespace, EOF, etc)
    • Formatting for TOML, YAML, markdown files
    • C++ linting & formatting with clang-format
    • Python linting with Ruff, formatting with Ruff, type checking with mypy, unit testing

GitHub Actions Refactoring

  • Updated ci.yml
    • Updated versions of actions/checkout and actions/setup-python
    • Added separate jobs for Python type checking and package installation testing
  • Created pre-commit.yml to run pre-commit checks in CI
  • Added dependabot.yml to keep GitHub Action versions up-to-date

Benefits

These changes offer several benefits:

  • Automated checks to catch code quality issues early in the development process
  • Consistent styling and best practices enforced across the codebase
  • Reduced back-and-forth in code reviews by addressing common issues automatically
  • Improved CI pipeline with dedicated jobs for different aspects of the project

Developers can run the pre-commit hooks locally to identify and fix problems before committing, leading to a smoother development experience and a more stable codebase.

Please review the changes and provide any feedback or suggestions. Let's work together to enhance our code quality and streamline our development process!

@azriel1rf
Copy link
Collaborator Author

azriel1rf commented May 2, 2024

I noticed that some files are currently blocked by the pre-commit hooks. To resolve these issues and ensure a smooth merge, I have made the necessary changes in separate pull requests:

  1. Improve Python Code Formatting and Quality #94

    • Addresses issues with python/setup.cfg
  2. Improve formatting of Algorithm.md and python/README.md #96

    • Fixes trailing whitespace in Documentation/Algorithm.md
    • Ensures proper end of file for python/README.md and Documentation/Algorithm.md
  3. Apply code formatting to C and C++ files #97

    • Removes trailing whitespace in C files
    • Fixes end of file issues in C and C++ files
    • Converts CRLF to LF line endings
    • Replaces tabs with spaces in C and C++ files
  4. Format markdown files #100

  5. Fix multiple empty lines #101

Please review and merge these pull requests first. Once they are merged, the pre-commit hooks should pass successfully for this pull request.

If there are any further issues or if you have any suggestions for improving the pre-commit hooks or GitHub Actions workflow, please let me know. I'm open to feedback and collaboration to enhance our development process.

Thank you for your attention to code quality and for setting up these valuable tools!

@azriel1rf azriel1rf force-pushed the setup_linter branch 3 times, most recently from 912c745 to ed6e1e8 Compare May 2, 2024 23:46
Copy link
Owner

@HenryRLee HenryRLee left a comment

Choose a reason for hiding this comment

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

Thanks for introducing this linter tool to the workflow. It looks awesome!

I have a question, is there a way to show the code diff in the workflow log output? Currently I cannot find the diff in this workflow log.

I don't expect many contributors would download pre-commit tool and run it locally. Of course, we can add some instructions in CONTRIBUTING.md to guide them, but it still creates additional barriers for them. So relying on the workflow logs may be an easier way for them. Well, if contributors are really stuck in passing the linter check, in the end, as maintainers we may have to step up to help them, e.g. show them the mistake in code review, or even commit a fix for them.

Also, we may add some more info in the CONTRIBUTING.md file, to ask contributors follow the Google C++ coding style guide, and the corresponding Python coding style guide (is it https://peps.python.org/pep-0008 ?).

@azriel1rf
Copy link
Collaborator Author

The pre-commit hooks failed with exit code 143 when executing clang-format on large files, as seen in the GitHub Actions logs:

Error: Process completed with exit code 143.

https://github.com/HenryRLee/PokerHandEvaluator/actions/runs/8934731565/job/24542074363?pr=95#step:4:145

This issue is likely caused by a crash due to insufficient resources, as discussed in actions/runner-images#6680.

To resolve this problem, I will:

  1. Reset the formatting on the affected large files to their original state.
  2. Update the pre-commit hook configuration to exclude these large files from linting and formatting.

By excluding the large files, we can ensure that the pre-commit hooks run successfully without encountering resource limitations.

@azriel1rf
Copy link
Collaborator Author

Thank you for sharing your thoughts and concerns regarding the pre-commit hooks and contributor experience.

I agree that we shouldn't force every contributor to use pre-commit locally, as it may create additional barriers for them. To make the process easier, I will enable verbosity for all pre-commit hooks in the GitHub Actions workflow. This will display the code diff and highlight the specific areas where the code fails to meet the style guidelines. Contributors can then refer to the workflow logs to identify and fix the issues without needing to run pre-commit locally.

Regarding the coding style guides, I suggest we update the CONTRIBUTING.md file to include the following information:

By providing clear guidelines in the CONTRIBUTING.md file, we can help contributors understand the project's coding style expectations and reduce the likelihood of style-related issues in their pull requests.

I appreciate your input and collaboration in improving the contributor experience.

@azriel1rf azriel1rf marked this pull request as draft May 3, 2024 06:58
@HenryRLee
Copy link
Owner

That sounds good! Thanks for the efforts!

@azriel1rf azriel1rf marked this pull request as ready for review May 4, 2024 03:38
@azriel1rf azriel1rf mentioned this pull request May 4, 2024
@azriel1rf azriel1rf force-pushed the setup_linter branch 3 times, most recently from 0d159a5 to c68bcdf Compare May 4, 2024 05:08
@azriel1rf
Copy link
Collaborator Author

After rebasing, the fiels not modified are excluded from pre-commit.

@azriel1rf
Copy link
Collaborator Author

After rebasing, the fiels not modified are excluded from pre-commit.

This was my mistaken.

@azriel1rf azriel1rf marked this pull request as draft May 4, 2024 05:17
@azriel1rf azriel1rf force-pushed the setup_linter branch 2 times, most recently from 8e5cd12 to bf7f775 Compare May 4, 2024 05:44
@azriel1rf
Copy link
Collaborator Author

With merging #100 and #101, the pre-commit successed.
https://github.com/HenryRLee/PokerHandEvaluator/actions/runs/8948554069/job/24581813056?pr=95
I will revert merges and push this branch again.

@azriel1rf azriel1rf marked this pull request as ready for review May 4, 2024 06:22
@azriel1rf azriel1rf force-pushed the setup_linter branch 3 times, most recently from 6112fab to 067c135 Compare May 4, 2024 06:32
@azriel1rf azriel1rf marked this pull request as draft May 4, 2024 07:07
@azriel1rf azriel1rf force-pushed the setup_linter branch 3 times, most recently from 844c7fd to 336a16c Compare May 4, 2024 11:36
@azriel1rf
Copy link
Collaborator Author

@HenryRLee I've made the following changes based on your review:

  • Added a git diff step in the pre-commit configuration to display the differences between the original and formatted code. This will provide contributors with clear guidance on where and how to format their code, even if they don't run pre-commit locally.

  • Updated the CONTRIBUTING.md file to include detailed explanations of the code styles, formatters, linters, building, and testing processes for both C++ and Python.

  • Added a section in CONTRIBUTING.md to explain the GitHub Actions workflow, including the various checks and tests performed for each pull request. This will give contributors a better understanding of the automated processes.

  • Expanded the contributing section in python/README.md to cover code styles, development setup, and the use of pre-commit hooks.

Please let me know if you have any further suggestions or concerns. I'm happy to improve the contribution experience for everyone involved.

Copy link
Owner

@HenryRLee HenryRLee left a comment

Choose a reason for hiding this comment

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

Looks great! The guidelines are very detail. Many thanks for your efforts!

@azriel1rf azriel1rf marked this pull request as ready for review May 5, 2024 11:12
@azriel1rf azriel1rf merged commit 9b05125 into develop May 5, 2024
12 checks passed
@azriel1rf azriel1rf deleted the setup_linter branch May 5, 2024 11:13
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.

2 participants