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

style: pre-commit basic config #1151

Merged
merged 2 commits into from
Oct 27, 2022

Conversation

henryiii
Copy link
Contributor

@henryiii henryiii commented May 24, 2022

This adds a basic pre-commit-config, a tox job running it (to help new contributors - usually you should run pre-commit directly to take advantage of it's start-up speed), and a GitHub job. Pre-commit is from Anthony Sottile, the developer behind flake8, pytest, tox, and other tools. You can read more about it at https://pre-commit.com or https://scikit-hep.org/developer/style. You can see it on many of the PyPA repos, like build, pip, pipx, cibuildwheel, etc. If you want, there's also a pre-commit.ci service that will auto-update your pre-commit file and auto-fix PRs, and is ultra-fast.

If you like this, I can add a few more checks, starting with the basics. MyPy can be moved to pre-commit as well. I won't add black or isort unless you ask for them, but I think you'll really like pyupgrade.

By the way, you can exclude files, even per-check, so let me know if there's any that intentionally unusual (like wrong file endings) for testing purposes. I can also ignore SVGs, etc, quite easily.

  • chore: add pre-commit basic checks
  • style: run pre-commit - basic checks

FYI, I mostly run pre-commit run -a like you might run nox or tox, but you can also run pre-commit install and have it run in "precommit hook mode", where it checks on commit unless you pass -n.

@erezsh
Copy link
Member

erezsh commented May 25, 2022

Why did it modify the SVGs?

MyPy can be moved to pre-commit as well

It sounds like a good fit for pre-commit. But moved from where?

pyupgrade looks nice! (with --keep-percent-format, because it doesn't support f-strings, it seems). Though I'm not a fan of pre-commits that change files beyond whitespace. But as long as it's a manual run, it's a nice addition!

Is there a way to split the pre-commit, so that checks run automatically (like with install), but modifications need to be run manually?

Another question - if the files get modified by the CI, who is signed on the commit? Disregard. It's better that the CI won't make any changes :)

start: "a"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be excluded! :)

@henryiii
Copy link
Contributor Author

Why did it modify the SVGs?

Probably for consistent end lines or extra whitespace.

But moved from where?

Currently it's sitting in tox.ini.

because it doesn't support f-strings, it seems

What doesn't support f-strings? Pyupgrade will convert % to f-strings if there's no possible output change and the expression in the f-string is very simple. I know of some other tools that are a bit more aggressive that can be used to convert the rest. f-strings are faster than % or .format1.

Though I'm not a fan of pre-commits that change files beyond whitespace

There's not that much you can do beyond whitespace and a few minor things in Python. We don't have an equivalent to Rubocop for Python. Most tools are just linters only.

But as long as it's a manual run, it's a nice addition

Pre-commit is just the name of the tool. It's not a very good name.

Is there a way to split the pre-commit, so that checks run automatically (like with install), but modifications need to be run manually?

You can set stages on all hooks. There is a manual hook stage, but you have to specify it when running pre-commit. I'm not sure if you can specify a "non-precommit" state that runs manually without adding --hook-stage manual. I'd generally not bother, and just don't use the precommit mode if you don't want it running. (As I mentioned, I rather rarely install and use it as a git precommit hook).

It's worth trying out, though, if you haven't. It's very well done - it handles partial checkings, for example, by copying to a temp directory and running there, etc. I'd recommend trying out everything on the page I shared, one at a time, eventually. I'll only be adding this (I was having issues with line endings on my last PR), pyupgrade, and maybe some linting checks - I'm really after improving the typing coverage a bit so I can measure the impact of mypyc. MyPy and Black both use mypypc to get nice speedups. Style formatting (Black, isort, etc) are a personal choice, and not a usually a good contribution unless asked for.

It's better that the CI won't make any changes

You can still activate pre-commit.ci, but leave changes off. We do that in pypa/build. You still get the fast checks & weekly updates.


I'll add a couple of exclusions, and repush, and we can go from there. Probably in an hour or two.

Footnotes

  1. Python 3.11 allows very simple % formatting to be as fast as f-strings.

@erezsh
Copy link
Member

erezsh commented May 25, 2022

Probably best to exclude SVGs.

Currently it's sitting in tox.ini.

Doesn't it make sense to have it in both?

Pyupgrade will convert % to f-strings

That would be great. When I tried, it converted them to .format() calls instead.

Style formatting (Black, isort, etc) are a personal choice

Good call. I'm not a fan of reformatters, and have bad experience with isort.

You can still activate pre-commit.ci, but leave changes off

Sounds good.

I'm also curious to know if the type info produces any speed-up, and by how much.

@henryiii
Copy link
Contributor Author

Probably best to exclude SVGs.

I'll also exclude .lark files in tests.

Doesn't it make sense to have it in both?

Sure, but it should be defined once (in pre-commit), and simply run from there via tox.

When I tried, it converted them to .format() calls instead

Ah, you forgot to pass --py36-plus. It tries to upgrade to 2.7 syntax without any target.

bad experience with isort.

Probably some time ago? It was reworked and it very good these days. And Black is great - once you get used to reading blacked code, it becomes really easy to read lots of code bases.

@henryiii henryiii force-pushed the henryiii/style/pre-commit-basic branch from 341d7b3 to ce7dd7b Compare May 25, 2022 13:17
@henryiii
Copy link
Contributor Author

henryiii commented May 25, 2022

Here's the report so you can see all changed files and why they were changed:

check for added large files..............................................Passed
check for case conflicts.................................................Passed
check for merge conflicts................................................Passed
check for broken symlinks............................(no files to check)Skipped
check toml...............................................................Passed
check yaml...............................................................Passed
debug statements (python)................................................Passed
fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing LICENSE
Fixing lark/parsers/lalr_interactive_parser.py
Fixing lark/parsers/lalr_parser.py
Fixing docs/requirements.txt
Fixing setup.py
Fixing examples/grammars/README.md
Fixing docs/visitors.rst
Fixing examples/standalone/README.md
Fixing docs/features.md
Fixing examples/advanced/py3to2.py
Fixing examples/advanced/error_handling.py
Fixing lark/parsers/earley_common.py
Fixing docs/ide/app/app.py
Fixing docs/ide/app/html5.py
Fixing docs/conf.py
Fixing examples/advanced/python2.lark
Fixing CHANGELOG.md
Fixing examples/advanced/error_reporting_earley.py
Fixing examples/composition/storage.lark
Fixing docs/ide/app/files.json
Fixing docs/_static/sppf/sppf.html
Fixing docs/philosophy.md
Fixing examples/advanced/error_reporting_lalr.py
Fixing test-requirements.txt
Fixing docs/tools.md
Fixing README.md
Fixing docs/recipes.md
Fixing docs/Makefile
Fixing examples/indented_tree.py
Fixing examples/composition/README.md
Fixing tests/test_lexer.py
Fixing .github/ISSUE_TEMPLATE/other.md
Fixing examples/composition/main.py
Fixing .gitignore
Fixing docs/ide/app/examples.py
Fixing tests/test_grammar.py
Fixing examples/advanced/templates.py
Fixing examples/standalone/json_parser_main.py
Fixing examples/tests/no_newline_at_end.lark
Fixing examples/advanced/reconstruct_python.py
Fixing lark/tools/standalone.py
Fixing lark/__pyinstaller/__init__.py
Fixing docs/classes.rst
Fixing examples/advanced/python_parser.py

mixed line ending........................................................Failed
- hook id: mixed-line-ending
- exit code: 1
- files were modified by this hook

examples/composition/README.md: fixed mixed line endings
examples/advanced/py3to2.py: fixed mixed line endings
CHANGELOG.md: fixed mixed line endings
examples/grammars/README.md: fixed mixed line endings

fix requirements.txt.....................................................Failed
- hook id: requirements-txt-fixer
- exit code: 1
- files were modified by this hook

Sorting docs/requirements.txt

trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing lark/parsers/lalr_interactive_parser.py
Fixing examples/advanced/py3to2.py
Fixing tests/test_python_grammar.py
Fixing lark/lark.py
Fixing lark/exceptions.py
Fixing lark/parsers/earley_common.py
Fixing examples/grammars/verilog.lark
Fixing lark/ast_utils.py
Fixing README.md
Fixing lark/parsers/earley.py
Fixing lark/parser_frontends.py
Fixing lark/visitors.py
Fixing docs/classes.rst
Fixing lark/grammars/python.lark
Fixing docs/how_to_develop.md
Fixing tests/test_cache.py
Fixing lark/tools/__init__.py
Fixing tests/test_parser.py

@henryiii
Copy link
Contributor Author

Also, if you rebase-and-merge or classic merge, then you can add the second commit's sha to your .git-ignore-revs file and GitHub will ignore that commit in history (blame) view (recent addition to GitHub).

@erezsh
Copy link
Member

erezsh commented May 25, 2022

Only thing I noticed was that the comment in docs/requirements.txt is now misplaced due to sorting it.

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
@henryiii henryiii force-pushed the henryiii/style/pre-commit-basic branch from ce7dd7b to 5236ff9 Compare May 25, 2022 14:05
erezsh added a commit that referenced this pull request Oct 27, 2022
@erezsh erezsh merged commit a885ca0 into lark-parser:master Oct 27, 2022
@erezsh
Copy link
Member

erezsh commented Oct 27, 2022

Thanks you for your contribution!

Sorry that it took so long to review. My mind's been on other projects.

@henryiii
Copy link
Contributor Author

No problem, my mind has been too. I'll pop back here once and a while and try to slowly work on more typing, still curios to try mypyc.

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