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

Expose prepare-commit-msg arguments as environment vars #2407

Merged
merged 1 commit into from Jun 11, 2022

Conversation

M-Whitaker
Copy link
Contributor

@M-Whitaker M-Whitaker commented May 28, 2022

Closes #2406.

This adds the ability to pass environ args that are not files to the prepare-commit-msg hook as defined by the git specification:

Git hook documentation

If you want some tests for this more than happy to think up some, however, I have tried to only add to existing code and use optional arguments so all existing tests are not affected.

@asottile
Copy link
Member

so it shouldn't treat them as arguments, pre-commit only deals in files

they should be environment variables to match up with the other hooks

@M-Whitaker
Copy link
Contributor Author

Ah! I see what you mean now...

so it shouldn't treat them as arguments, pre-commit only deals in files

they should be environment variables to match up with the other hooks

I have changed it accordingly.

pre_commit/commands/hook_impl.py Outdated Show resolved Hide resolved
pre_commit/main.py Outdated Show resolved Hide resolved
pre_commit/main.py Outdated Show resolved Hide resolved
@M-Whitaker
Copy link
Contributor Author

M-Whitaker commented May 29, 2022

This was the most elegant way I thought of to keep this underneath the PEP8 line length. Maybe you have an opinion on this...

if args.prepare_commit_message_source:
         environ['PRE_COMMIT_COMMIT_MSG_SOURCE'] \
             = args.prepare_commit_message_source

I have implemented all the other name changes as you suggested.

In regards to the tests:

Do you usually extend the existing tests i.e. test_prepare_commit_msg_hook or do you want one for each different possible argument passed from the hook?

@asottile
Copy link
Member

This was the most elegant way I thought of to keep this underneath the PEP8 line length. Maybe you have an opinion on this...

if args.prepare_commit_message_source:
         environ['PRE_COMMIT_COMMIT_MSG_SOURCE'] \
             = args.prepare_commit_message_source

I have implemented all the other name changes as you suggested.

In regards to the tests:

Do you usually extend the existing tests i.e. test_prepare_commit_msg_hook or do you want one for each different possible argument passed from the hook?

even pep8 suggests using parens over backslashes:

The preferred way of wrapping long lines is by using Python’s implied line continuation inside parentheses, brackets and braces.

and yeah there'll need to be new tests -- whatever is needed to demonstrate the change should be sufficient

@M-Whitaker M-Whitaker requested a review from asottile May 31, 2022 23:06
@asottile asottile changed the title Add extra args to prepare-commit-msg Expose prepare-commit-msg arguments as environment vars Jun 11, 2022
Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

@asottile asottile merged commit 4738d06 into pre-commit:main Jun 11, 2022
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

prepare-commit-msg Not passing all parameters
2 participants