Skip to content

Make pyastgrep usable as a pre-commit hook - #27

Closed
ashrub-holvi wants to merge 1 commit into
spookylukey:masterfrom
ashrub-holvi:master
Closed

Make pyastgrep usable as a pre-commit hook#27
ashrub-holvi wants to merge 1 commit into
spookylukey:masterfrom
ashrub-holvi:master

Conversation

@ashrub-holvi

Copy link
Copy Markdown

Hi,

I played a bit for make pyastgrep usable as pre-commit hook, not really sure how practical is to use it this ways, but I guess it's cheap to have it and give people chance to try, at least for simple cases it should be good.

@holvi-noor holvi-noor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good to me, lets try 👍

@spookylukey

Copy link
Copy Markdown
Owner

Thanks so much for this, it's an interesting approach.

I've documented the patterns I use for this kind of thing here: https://lukeplant.me.uk/blog/posts/pyastgrep-and-custom-linting/

There are some significant issues with pyastgrep for linting, especially that non-matches can start failing silently very easily when you change Python version, due to changes in Python's AST, or just because you didn't check the XPath expression properly - the XPath is tricky enough to need its own tests. That's why I prefer the method in the post that uses explicit "can I find the needle" checks, with example of what exactly the needle looks like.

Regarding the example in the PR:

            name: No usage of legacy_function_name
            args: ['.//FunctionDef[@name="legacy_function_name"]']

This actually checks for a definition def legacy_function_name(), not usages of it. Usages are much harder to check exhaustively, and for this kind of thing I use Semgrep. So maybe the example serves as an illustration of why other tools are better for linting?

I'd be interested in your feedback!

@ashrub-holvi

Copy link
Copy Markdown
Author

https://lukeplant.me.uk/blog/posts/pyastgrep-and-custom-linting/

Thanks!

This actually checks for a definition def legacy_function_name(), not usages of it.

yes, not carefully enough prepared example

I'd be interested in your feedback!

I think this

There are some significant issues with pyastgrep for linting, especially that non-matches can start failing silently very easily when you change Python version, due to changes in Python's AST, or just because you didn't check the XPath expression properly

is a quite good explanation, so closing it, thank you!

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