lark icon indicating copy to clipboard operation
lark copied to clipboard

style: pre-commit basic config

Open henryiii opened this issue 3 years ago • 7 comments

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.

henryiii avatar May 24 '22 22:05 henryiii

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 :)

erezsh avatar May 25 '22 11:05 erezsh

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 .format[^1].

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.

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

henryiii avatar May 25 '22 12:05 henryiii

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.

erezsh avatar May 25 '22 12:05 erezsh

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 avatar May 25 '22 13:05 henryiii

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 avatar May 25 '22 13:05 henryiii

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).

henryiii avatar May 25 '22 13:05 henryiii

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

erezsh avatar May 25 '22 13:05 erezsh

Thanks you for your contribution!

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

erezsh avatar Oct 27 '22 18:10 erezsh

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.

henryiii avatar Oct 27 '22 18:10 henryiii