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

1928 - allow for customised naming of hooks and sort hooks for execution #1956

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

david-abn
Copy link
Contributor

@david-abn david-abn commented Oct 4, 2023

Issue
#1928

About
This PR will allow cookiecutter users to create multiple scripts for any hook type. It will also sort them.
i.e. pre_prompt_1, pre_prompt_2

Testing
Created post_gen_project_2.sh in new cookiecutter template with

#!/bin/bash

touch test_post_gen_project_2

Verified that this file is created, and the files are run in sorted order.

Documentation
Updated documentation with this new functionality

@ericof
Copy link
Member

ericof commented Nov 16, 2023

@david-abn I like this PR. Could you add a new test to show this is working?

@ericof ericof added the feature This issue/PR relates to major feature request. label Nov 16, 2023
@ericof ericof self-requested a review November 16, 2023 19:33
@david-abn
Copy link
Contributor Author

david-abn commented Dec 22, 2023

@david-abn I like this PR. Could you add a new test to show this is working?

Added new commit to update unit test. Thank you

:param hook_file: The hook file to consider for validity
:param hook_name: The hook to find
:return: The hook file validity
"""
filename = os.path.basename(hook_file)
basename = os.path.splitext(filename)[0]
matching_hook = basename == hook_name
supported_hook = basename in _HOOKS
matching_hook = hook_name in basename
Copy link

Choose a reason for hiding this comment

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

I think it should be a requirement that file names start with the hook identifier to avoid unintended execution of files. For example, without this constraint, a file like utils_for_pre_gen_project.py could run as a hook unintentionally.

Suggested change
matching_hook = hook_name in basename
matching_hook = basename.startswith(hook_name)

matching_hook = basename == hook_name
supported_hook = basename in _HOOKS
matching_hook = hook_name in basename
supported_hook = [i for i in _HOOKS if i in basename]
Copy link

Choose a reason for hiding this comment

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

The row above already tests that the file is a requested hook type.
It should now be enogh to test if the requested hook type is supported.

Suggested change
supported_hook = [i for i in _HOOKS if i in basename]
supported_hook = hook_name in _HOOKS

Alternatively the any() function could be utilized to get a boolean value right away

Suggested change
supported_hook = [i for i in _HOOKS if i in basename]
supported_hook = any(basename.startswith(supported_hook_name) for supported_hook_name in _HOOKS)

@@ -59,14 +59,22 @@ def test_run_python_hooks():
def test_run_python_hooks_cwd():
"""Verify pre and post generation python hooks executed and result in current dir.

Check that multiple hooks can be created with python_post and python_post_2.
python_post_2 should be created before python_post to verify it is sorted.
Copy link

Choose a reason for hiding this comment

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

Suggested change
python_post_2 should be created before python_post to verify it is sorted.
python_post_2 should be created after python_post to verify it is sorted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to major feature request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants