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

Revision filename for post_write_hooks options passed in wrong position #819

Closed
maresb opened this issue Mar 26, 2021 · 1 comment
Closed
Labels
bug Something isn't working use case not quite a feature and not quite a bug, something we just didn't think of

Comments

@maresb
Copy link
Contributor

maresb commented Mar 26, 2021

Describe the bug

While trying to run pre-commit as a post-write hooks, I encountered a failure. My config was

hooks = pre-commit
pre-commit.type = console_scripts
pre-commit.entrypoint = pre-commit
pre-commit.options = run --files

The failure occurred because the pre-commit command was being run as

pre-commit /path/to/revision_file.py run --files

instead of

pre-commit run --files /path/to/revision_file.py

Expected behavior

The documentation states:

The path of the revision file is sent as a single positional argument to the script after the options.

However, checking the code, this is not the case, as it is being passed as the first argument.

Additional note

The command line options are being split using str.split(), but for command line options it's much better to use shlex.split() since that properly handles things like quoting.

PR incoming...

@maresb maresb added the requires triage New issue that requires categorization label Mar 26, 2021
maresb added a commit to maresb/alembic that referenced this issue Mar 26, 2021
@CaselIT CaselIT added bug Something isn't working use case not quite a feature and not quite a bug, something we just didn't think of and removed requires triage New issue that requires categorization labels Mar 27, 2021
sqlalchemy-bot pushed a commit that referenced this issue Mar 27, 2021
I include a token called `REVISION_SCRIPT_FILENAME` which can be used in the `options` setting of a post-write hook. This token is replaced by the filename of the revision script, allowing the filename to be passed to the post-write hook in a flexible manner. Thus I can do:

```ini
[post_write_hooks]
hooks = pre-commit
pre-commit.type = console_scripts
pre-commit.entrypoint = pre-commit
pre-commit.options = run --files REVISION_SCRIPT_FILENAME
```

Note that the existing behavior is to prepend `REVISION_SCRIPT_FILENAME` to the argument list. In order to preserve backwards compatibility, this is done when `REVISION_SCRIPT_FILENAME` is not present.

### Description
* Implement the `REVISION_SCRIPT_FILENAME` token.
* Switch from `str.split()` to `shlex.split()` for robust command line argument processing.
* Insert `REVISION_SCRIPT_FILENAME` in appropriate locations in the docs and templates.
* Properly document that `REVISION_SCRIPT_FILENAME` is prepended instead of appended to the argument list.
* Refactored existing "test of `black` as a post-write hook" in order to avoid code duplication.
* Add a test for `REVISION_SCRIPT_FILENAME` and shlex.

### Checklist
<!-- go over following points. check them with an `x` if they do apply, (they turn into clickable checkboxes once the PR is submitted, so no need to do everything at once)

-->

This pull request is:

- [X] A new feature implementation
	- please include the issue number, and create an issue if none exists, which must
	  include a complete example of how the feature would look.
	- Please include: `Fixes: #<issue number>` in the commit message
	- please include tests.

Fixes issue #819

Closes: #820
Pull-request: #820
Pull-request-sha: 73dd0a4

Change-Id: Ibfc677d59086703872d7cfd5c2da32bd0220a25f
@CaselIT CaselIT closed this as completed Mar 27, 2021
@CaselIT
Copy link
Member

CaselIT commented Mar 27, 2021

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working use case not quite a feature and not quite a bug, something we just didn't think of
Projects
None yet
Development

No branches or pull requests

2 participants