Add option to scripts-dev/lint.sh to only lint files changed since the last git commit

[anoadragon453]

This PR makes several changes to the ./scripts-dev/lint.sh script, which lints the codebase with a number of tools:

• Adds usage information, with -h flag to show it. Otherwise it will show when providing an unknown flag.
• Adds option -d which will check both staged and unstaged files that have changed since the last commit and add them to the list of files to lint.
  • Note that only files without an extension, or with a .py extension will be allowed. This prevents editing bash scripts causing the linters to break on non-python files.
• Improves the print-out of which files/directories are being linted.

The result is:

~/code/synapse anoa/module_api_send_events *63 +1 ?19 ❯ vim tests/module_api/test_api.py
~/code/synapse anoa/module_api_send_events *63 +1 ?19 ❯ ./scripts-dev/lint.sh -d
Linting these paths:
-  "tests/module_api/test_api.py"

+ isort tests/module_api/test_api.py
+ python3 -m black tests/module_api/test_api.py
reformatted tests/module_api/test_api.py
All done! ✨ 🍰 ✨
1 file reformatted.
+ ./scripts-dev/config-lint.sh
+ flake8 tests/module_api/test_api.py
tests/module_api/test_api.py:68:9: F841 local variable 'room_id' is assigned to but never used
~/code/synapse anoa/module_api_send_events *63 +1 !1 ?19 ❯

There's a lot of bash going on in this. Feel free to ping in #synapse-dev if you need help understanding any of the syntax.

[anoadragon453]

I'm also thinking it might be nice to have an option that checks for changed files against a given branch name, but that could be in a separate PR.

[clokep]

I unfortunately find that output much harder to read. I'd be curious what sort of speed-up you're seeing from this? This script takes < 5 seconds to run for me, which seems reasonable.

Would it make more sense to first look at running on multiple cores (if this isn't done automatically)? It would also help if we didn't have isort + black fighting. I saw that newer versions of isort have a black compatible mode.

[anoadragon453]

This script takes < 5 seconds to run for me, which seems reasonable.

I'm jealous, it takes about 15-20 seconds to run here. flake8 is definitely the one that takes the longest. I think I tried running it on multiple cores before with little success, but can experiment with it tomorrow.

The speed up is massive. Since I only edit a few files (max 5-10) per PR, the script completes nearly instantly.

I unfortunately find that output much harder to read.

Is that due to the fact it's printing out each command (set -x) or the fact that it's printing out each filepath that it's linting? I mostly left the set -x in to help with debugging, but it's simple to take it out again. I could also remove the quotes from the filepaths, but again that was for debugging (but likely also unnecessary).

For reference, the current output is:

~/code/synapse anoa/module_api_send_events *64 ?19 ❯ ./scripts-dev/lint.sh tests/module_api/test_api.py
Linting these locations: tests/module_api/test_api.py
All done! ✨ 🍰 ✨
1 file left unchanged.
~/code/synapse anoa/module_api_send_events *64 ?19 ❯  

and yes, I agree it's much cleaner.

[clokep]

Is that due to the fact it's printing out each command (set -x) or the fact that it's printing out each filepath that it's linting? I mostly left the set -x in to help with debugging, but it's simple to take it out again. I could also remove the quotes from the filepaths, but again that was for debugging (but likely also unnecessary).

I think mostly due to printing out each file. Including the calls to flake8, etc. isn't that much extra.

[anoadragon453]

Note that it'll just print out the files and directories it'll scan, rather than printing every file it will scan.

I could switch it back to the one-liners separated by spaces, so it would look like:

(env) ➜  synapse git:(anoa/module_api_send_events) ✗ ../lint.sh -d
Linting these paths: synapse/handlers/message.py synapse/module_api/__init__.py

+ isort synapse/handlers/message.py synapse/module_api/__init__.py
+ python3 -m black synapse/handlers/message.py synapse/module_api/__init__.py
All done! ✨ 🍰 ✨
2 files left unchanged.
+ ./scripts-dev/config-lint.sh
+ flake8 synapse/handlers/message.py synapse/module_api/__init__.py
(env) ➜  synapse git:(anoa/module_api_send_events) ✗ 

which admittedly looks pretty good.

[clokep]

@anoadragon453 I think that's quite a bit more readable.

[anoadragon453]

Changes made then!

[clokep]

This seems to break if you use the -d flag on a clean repo. (isort shows help text, etc.)

[anoadragon453]

This seems to break if you use the -d flag on a clean repo.

Good spot!

stale

[richvdh]

seems generally fine to me. apart from the setup.py thing, all of this is just stylistic thoughts which don't matter too much. I see it's already been around the review loop a few times and don't wish to hold it up further unless you're feeling keen.

[richvdh]

I think you should stick to one of [ ] and [[ ]] :-p

(There is a difference. I can never remember what it is without looking it up.)

[anoadragon453]

[ ... ] is POSIX-compliant, whereas [[ ... ]] supports fancy bash stuff like &&, ||, ~= etc. 1

https://stackoverflow.com/questions/13542832/difference-between-single-and-double-square-brackets-in-bash

We can't make the whole script POSIX-compliant however due to our heavy use of arrays, which are undefined in POSIX.

I have changed things to be more consistent in e1abece7c though.

[richvdh]

it feels to me like trying to mix -d and command-line parameters should be rejected: mixing the two seems confusing.

[anoadragon453]

I suppose there's not much use-case for it... though I'm not sure if we explicitly need to block it either. There may a random case where someone might want to do so.

[richvdh]

lgtm assuming CI is happy