Skip to content

[CI] Add three pre-commit pygrep hooks #12933

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.

Sign up for GitHub

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

Open
jbampton wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[CI] Add three pre-commit pygrep hooks #12933

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3b54c62
[CI] Add three pre-commit pygrep hooks
jbampton Mar 31, 2026
72a63d3
Merge branch 'main' into add-pre-commit-pygrep-hooks
jbampton Aug 12, 2026
74ad958
Merge branch 'main' into add-pre-commit-pygrep-hooks
jbampton Sep 9, 2026
69c15b3
Pin hook to sha
jbampton Sep 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,19 @@ repos:
files: ^(LICENSE|NOTICE)$|README$|\.(bat|cfg|config|cs|css|erb|gitignore|header|in|install|java|md|properties|py|rb|rc|sh|sql|svg|te|template|txt|ucls|vue|xml|xsl|yaml|yml)$|^cloud-cli/bindir/cloud-tool$|^debian/changelog$
args: [--markdown-linebreak-ext=md]
exclude: ^services/console-proxy/rdpconsole/src/test/doc/freerdp-debug-log\.txt$
- repo: https://github.com/pre-commit/pygrep-hooks
rev: 3a6eb0fadf60b3cccfd80bad9dbb6fae7e47b316 # frozen: v1.10.0
hooks:
- id: python-check-mock-methods
Comment thread
DaanHoogland marked this conversation as resolved.
Resolved
name: run check for not-real mock methods
description: Prevent common mistakes of assert mck.not_called(), assert mck.called_once_with(...) and mck.assert_called
Comment on lines +193 to +194
- id: python-use-type-annotations
name: run check for type annotations not comments
description: Enforce that python3.6+ type annotations are used instead of type comments
exclude: ^scripts/vm/hypervisor/xenserver/xenserver56/InterfaceReconfigure\.py$

@Damans227 Damans227 Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this one file skipped? I looked and it has no type comment in it. It just has a normal sentence that starts with the word "type". So any comment written like that in future will fail and end up added to this list. Is that ok?

@Damans227 Damans227 Oct 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could we reword the comment instead of skipping the whole file? Line 628 of InterfaceReconfigure.py says "# type parameter is a string...". Changing it to "# The type parameter is a string..." stops the match, and then the exclude can go:

Suggested change
exclude: ^scripts/vm/hypervisor/xenserver/xenserver56/InterfaceReconfigure\.py$

- id: text-unicode-replacement-char

@Damans227 Damans227 Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This runs over every file in the repo. Is there a chance some test data has one of these characters on purpose?

@Damans227 Damans227 Oct 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Checked, no tracked file has this character today, so it passes clean. Resolving.

@DaanHoogland DaanHoogland Oct 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Damans227 , so the exclude can be removed as well?

@Damans227 Damans227 Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@DaanHoogland yes, once line 628 of InterfaceReconfigure.py is reworded the exclude can go. @jbampton can you make both changes?

name: run check for no unicode replacement char
description: Forbid files which have a UTF-8 Unicode replacement character
- repo: https://github.com/codespell-project/codespell
rev: 57b21406f092110c18776e39b0bda50d37c945c8 # frozen: v2.4.3
hooks:
Expand Down
Loading