Skip to content

Add pre-commit#54

Closed
rruuaanng wants to merge 1 commit intodiofant:masterfrom
rruuaanng:add-pre-hook
Closed

Add pre-commit#54
rruuaanng wants to merge 1 commit intodiofant:masterfrom
rruuaanng:add-pre-hook

Conversation

@rruuaanng
Copy link
Copy Markdown
Contributor

No description provided.

@rruuaanng
Copy link
Copy Markdown
Contributor Author

As far as I know, only clang-format currently supports formatting of C. Personally, I think we don't need it, just like cpython doesn't use it yet.

@rruuaanng
Copy link
Copy Markdown
Contributor Author

It's Looks fine!

PS E:\code\github\python-gmp> git commit -m 'Add pre-commit'
Run ruff on tests/...................................(no files to check)Skipped
Sphinx Lint..........................................(no files to check)Skipped
[add-pre-hook 8ba3d2c] Add pre-commit
 2 files changed, 27 insertions(+)
 create mode 100644 .github/workflows/linter.yml
 create mode 100644 .pre-commit-config.yaml

@rruuaanng
Copy link
Copy Markdown
Contributor Author

cc @skirpichev Look this ;)

@rruuaanng
Copy link
Copy Markdown
Contributor Author

What C style do you prefer? GNU style? or kernel style? Maybe we need to write our own clang-format, but I want to hear your desired style for this project.

@skirpichev skirpichev marked this pull request as draft December 8, 2024 02:46
@skirpichev
Copy link
Copy Markdown
Member

What C style do you prefer? GNU style? or kernel style?

As this is a Python extension so far, I would choose PEP 7 style.

Please note, that pythoncapi_compat.h shouldn't be changed.

@rruuaanng
Copy link
Copy Markdown
Contributor Author

rruuaanng commented Dec 8, 2024

Alright, I will complete this PR in the near future. Currently, CPython does not strictly enforce PEP 7, as clang often cannot accurately describe it. However, I will try to make sure the code complies with PEP7 as much as possible, which may take some time. Please do not rush me. Especially to launch PR without notice, I need to reiterate this point.

@rruuaanng rruuaanng force-pushed the add-pre-hook branch 2 times, most recently from f4a118c to 57f73e2 Compare December 8, 2024 10:51
@rruuaanng rruuaanng marked this pull request as ready for review December 8, 2024 10:53
@rruuaanng
Copy link
Copy Markdown
Contributor Author

Let us rejoice, now we have formatting tools!

@rruuaanng
Copy link
Copy Markdown
Contributor Author

@skirpichev Please review them!

@skirpichev skirpichev self-requested a review December 8, 2024 12:54
Copy link
Copy Markdown
Member

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

So, some comments.

Sorry, I'm not sure how to improve things.

Comment thread main.c Outdated
Comment thread main.c Outdated
Comment thread main.c Outdated
Comment thread main.c Outdated
Comment thread main.c Outdated
Comment thread main.c Outdated
Comment thread main.c Outdated
@skirpichev skirpichev marked this pull request as draft December 9, 2024 05:57
@rruuaanng
Copy link
Copy Markdown
Contributor Author

Maybe we should remove the style check of C. Clang cannot strictly implement PEP7, because it combines multiple practices.

And CPython has no requirements for this, and only gives change requirements at the review stage.

@rruuaanng
Copy link
Copy Markdown
Contributor Author

I think it should be removed. At least there are only you and me participating in the current project. We just need to abide by it. @skirpichev

@rruuaanng rruuaanng marked this pull request as ready for review December 10, 2024 03:38
@skirpichev skirpichev self-requested a review December 12, 2024 08:07
Copy link
Copy Markdown
Member

@skirpichev skirpichev left a comment

Choose a reason for hiding this comment

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

I think should also check *.yml and pyproject.toml.

- uses: actions/setup-python@v5
with:
python-version: "3.x"
- uses: pre-commit/action@v3.0.1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's readme says: "this action is in maintenance-only mode". I don't think we should depend on obsoleted project.

Comment thread .pre-commit-config.yaml
- id: ruff
name: Run ruff on tests/
args: [--exit-non-zero-on-fix]
files: ^tests/
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should check all files, like current "test" action does. BTW, ruff test in the test.yml now redundant and should be removed.

@rruuaanng
Copy link
Copy Markdown
Contributor Author

Valuable suggestions, I will revise them.

@skirpichev skirpichev marked this pull request as draft December 14, 2024 03:39
@skirpichev
Copy link
Copy Markdown
Member

See #82

@skirpichev skirpichev closed this Dec 14, 2024
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