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 configuration including black and flake8 #45

Closed
wants to merge 7 commits into from

Conversation

ottobackwards
Copy link
Contributor

same as zkg
initial run

@ckreibich
Copy link
Member

Okay so the cause of the test failures is that reformatting the code pulled pylint instructions on a separate line, so pylint now fails and errors out. That's the reason for this ...

  /project/zeekscript/formatter.py:196:38: E1131: unsupported operand type(s) for | (unsupported-binary-operation)

... which in turn is likely the cause of the test failure.

You also need to ensure we keep git blame working. Take another look at what we did in the individual commits in zkg, see here and in particular this:

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

Also please separate the pre-commit tool buildup so it's easier to see in the commit hierarchy what changes resulted.

Nusthell: I meant "what we did in zkg" a bit more literally. :-)

@ottobackwards ottobackwards changed the title add pre-commit configuration [DRAFT] add pre-commit configuration Jan 26, 2023
@ottobackwards ottobackwards marked this pull request as draft January 26, 2023 22:35
@ottobackwards ottobackwards changed the title [DRAFT] add pre-commit configuration add pre-commit configuration including black and flake8 Jan 26, 2023
@bbannier
Copy link
Member

I have opened #50 to add a pre-commit setup similar to the one added here, though I didn't add flake8 hooks since they seemed to produce nothing interesting (I checked with the config from this PR).

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