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

Handle empty hook file or other OSErrors #729

Merged
merged 3 commits into from Dec 8, 2016
Merged

Handle empty hook file or other OSErrors #729

merged 3 commits into from Dec 8, 2016

Conversation

jcarbaugh
Copy link
Contributor

Took a guess at the right way to test based on the existing tests. Let me know if anything needs to be changed!

Fixes #632.

@codecov-io
Copy link

codecov-io commented Jun 4, 2016

Current coverage is 99.66%

Merging #729 into master will decrease coverage by 0.32%

  1. File ...ookiecutter/utils.py (not in diff) was modified. more
    • Misses +2
    • Partials 0
    • Hits -2
@@           master       #729   diff @@
========================================
  Files          13         13          
  Lines         583        589     +6   
  Methods         0          0          
  Messages        0          0          
  Branches        0          0          
========================================
+ Hits          583        587     +4   
- Misses          0          2     +2   
  Partials        0          0          

Sunburst

Powered by Codecov. Last updated by 124c570...7b905c9

@jcarbaugh
Copy link
Contributor Author

Argh... this error might be a *nix specific thing as the same error does not appear to be thrown on Windows. The tests are failing with DID NOT RAISE because Windows will always run a script with the shell, not relying on a shebang to exist. I'm going to skip that test on Windows.

@audreyfeldroy
Copy link
Member

@jcarbaugh I think it looks good, but I'd like a second pair of eyes on this before merging.

Can someone who has subprocess experience give this a second review? Just comment on the diff with anything that needs improvement.

@audreyfeldroy audreyfeldroy added please-help This issue/PR contain help request to cookiecutter users or first-time contributors needs-merge needs-review PR Only: This PR require review from other developer labels Jun 7, 2016
@pytest.mark.usefixtures('clean_system', 'remove_additional_folders')
def test_empty_hooks():

# OSError.errno=8 is not thrown on Windows when the script is emtpy
Copy link
Contributor

Choose a reason for hiding this comment

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

empty

@michaeljoseph
Copy link
Contributor

Looks fine to me, I picked a few nits though @jcarbaugh.

@hackebrot hackebrot mentioned this pull request Nov 25, 2016
10 tasks
@hackebrot hackebrot added this to the 1.5.0 Alfajor milestone Nov 26, 2016
@michaeljoseph
Copy link
Contributor

@jcarbaugh 🔔

hackebrot added a commit to hackebrot/cookiecutter that referenced this pull request Dec 2, 2016
@hackebrot hackebrot mentioned this pull request Dec 2, 2016
@hackebrot hackebrot merged commit 1641851 into cookiecutter:master Dec 8, 2016
hackebrot added a commit that referenced this pull request Dec 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review PR Only: This PR require review from other developer please-help This issue/PR contain help request to cookiecutter users or first-time contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants