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 pre-commit config #151

Merged
merged 7 commits into from
Jan 10, 2023
Merged

Add pre-commit config #151

merged 7 commits into from
Jan 10, 2023

Conversation

bbannier
Copy link
Member

@bbannier bbannier commented Jan 9, 2023

No description provided.

@bbannier bbannier self-assigned this Jan 9, 2023
setup.py Fixed Show resolved Hide resolved
setup.py Fixed Show fixed Hide fixed

if not on_rtd:
# only import and set the theme if we're building docs locally
import sphinx_rtd_theme
html_theme = 'sphinx_rtd_theme'

html_theme = "sphinx_rtd_theme"

Check notice

Code scanning / CodeQL

Unused global variable

The global variable 'html_theme' is not used.
Copy link
Member Author

Choose a reason for hiding this comment

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

Since Sphinx seems to use magic variables, I am unsure what pattern to use to suppress this warning. Might be worthwhile just manually dismissing this whenever we make an edit to this file. That way we'd still get CodeQL analysis everywhere else (where we are unlikely to use such weird patterns).

@bbannier bbannier force-pushed the topic/bbannier/pre-commit branch from a585d7f to 6aab9b6 Compare January 9, 2023 18:41
@bbannier bbannier marked this pull request as ready for review January 9, 2023 18:42
@bbannier bbannier requested a review from ckreibich January 9, 2023 18:42
Copy link
Member

@ckreibich ckreibich left a comment

Choose a reason for hiding this comment

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

I look forward to having this in there. One request Benjamin: could you introduce a .git-blame-ignore-revs file covering the reformatting commit, so we keep git blame working? We have that in Zeek too.

https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html#avoiding-ruining-git-blame

.github/workflows/pre-commit.yml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
@bbannier bbannier force-pushed the topic/bbannier/pre-commit branch from 6aab9b6 to e2e83e0 Compare January 10, 2023 08:58
.git-blame-ignore-revs Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
doc/ext/sphinxarg/ext.py Outdated Show resolved Hide resolved
@bbannier bbannier force-pushed the topic/bbannier/pre-commit branch from e2e83e0 to 2b0f8e7 Compare January 10, 2023 11:17
Even though I am unsure this can actually lead to issues, this was
flagged by CodeQL. Implementing a proper workaround instead of
continously whitelisting these instances.
@bbannier bbannier force-pushed the topic/bbannier/pre-commit branch from 2b0f8e7 to 4a28a69 Compare January 10, 2023 11:27
zeekpkg/_util.py Outdated Show resolved Hide resolved
@bbannier bbannier requested review from ckreibich and awelzel January 10, 2023 13:26
Copy link
Contributor

@awelzel awelzel left a comment

Choose a reason for hiding this comment

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

Approving based on skimming through and I like the idea.

@ckreibich ckreibich merged commit b218122 into master Jan 10, 2023
@ckreibich ckreibich deleted the topic/bbannier/pre-commit branch January 10, 2023 20:58
This was referenced Jan 13, 2023
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.

3 participants