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

Allow setting parameters with no defaults #588

Merged
merged 3 commits into from Dec 3, 2015

Conversation

johtso
Copy link
Contributor

@johtso johtso commented Nov 11, 2015

Fixes #587

@codecov-io
Copy link

Current coverage is 99.22%

Merging #588 into master will increase coverage by +0.01% as of 48b1464

@@            master    #588   diff @@
======================================
  Files           12      12       
  Stmts          511     513     +2
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            507     509     +2
  Partial          0       0       
  Missed           4       4       

Review entire Coverage Diff as of 48b1464


Uncovered Suggestions

  1. +0.39% via ...ookiecutter/utils.py#27...28
  2. +0.39% via ...okiecutter/config.py#19...20

Powered by Codecov. Updated on successful CI builds.

@pydanny
Copy link
Member

pydanny commented Nov 12, 2015

@johtso, thank you for your pull request. 🍪 😎

Could you add a test for the condition? While this is technically covered by tests (https://codecov.io/github/audreyr/cookiecutter/cookiecutter/prompt.py?ref=bc3cb0ed034bde485d963ee9b50217cbd29ee069), that you added code that's a false positive.

@johtso
Copy link
Contributor Author

johtso commented Nov 13, 2015

@pydanny will do!

@hackebrot
Copy link
Member

Hi @johtso! 👋

Could you please explain why you would have a variable with a default that is null in your template? An empty string should work perfectly fine if you do not want to specify a particular string value.

raw_var = str(raw_var)
from_string.assert_called_once_with(raw_var)
# Make sure that non None non str variables are conerted beforehand
if raw_var is not None:
Copy link
Member

Choose a reason for hiding this comment

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

This counteracts the extra test parameter! 😟

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, because the assertion doesn't apply anymore (there's still the assertion above checking the return value).

I could assert that it's not called I suppose..

This test does feel like it's basically replicating the same logic as the function it's testing.. but that's probably hard to avoid with such small functions.

@johtso
Copy link
Contributor Author

johtso commented Nov 13, 2015

@hackebrot the idea was that sometimes there is no sensible default for a value and you want to require that the user supplies one.

Even if that's not a particularly useful use-case, it's definitely less surprising than converting null into 'None'.

@hackebrot
Copy link
Member

Hey @johtso can you please rebase this guy? We sorted out the issues with AppVeyor and I'd like to see the test results for windows. Thank you 🙇

@hackebrot hackebrot mentioned this pull request Nov 29, 2015
@johtso
Copy link
Contributor Author

johtso commented Nov 30, 2015

@hackebrot done 😸

if not isinstance(raw_var, basestring):
raw_var = str(raw_var)
from_string.assert_called_once_with(raw_var)
# Make sure that non None non str variables are conerted beforehand
Copy link
Member

Choose a reason for hiding this comment

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

This sentence seems wrong 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions welcome! Looks like I kept a typo too..

pydanny added a commit that referenced this pull request Dec 3, 2015
Allow setting parameters with no defaults
@pydanny pydanny merged commit b879c8e into cookiecutter:master Dec 3, 2015
@pydanny
Copy link
Member

pydanny commented Dec 3, 2015

Grats on your first pull request here @johtso. Welcome aboard! 🚢 🚋

@johtso
Copy link
Contributor Author

johtso commented Dec 3, 2015

Cheers @pydanny

@johtso johtso deleted the patch-1 branch December 3, 2015 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants