Skip to content
This repository has been archived by the owner on Nov 16, 2023. It is now read-only.

PR: Remove redundant variables #7

Closed

Conversation

rlaverde
Copy link
Member

Remove project_name and repo_name, and use instead plugin_name modifications.

@rlaverde rlaverde requested a review from goanpeca January 17, 2017 02:26
@@ -43,11 +43,11 @@ dependencies:
test:
override:
# Check Python style
- $CMD_LINT_CIOCHECK {{ cookiecutter.repo_name }} -dt: # note the colon
- $CMD_LINT_CIOCHECK spyder-{{ cookiecutter.plugin_name.lower().replace(' ', '_') }} -dt: # note the colon
Copy link
Member

Choose a reason for hiding this comment

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

there is an error there should be _ not -

parallel: true

# Run Python tests
- $CMD_TEST_PYTEST {{ cookiecutter.repo_name }} --cov={{ cookiecutter.repo_name }}: # note the colon parallel: true
- $CMD_TEST_PYTEST spyder-{{ cookiecutter.plugin_name.lower().replace(' ', '_') }} --cov=spyder-{{ cookiecutter.plugin_name.lower().replace(' ', '_') }}: # note the colon parallel: true
Copy link
Member

Choose a reason for hiding this comment

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

same here

@goanpeca goanpeca added this to the v0.1 milestone Jan 17, 2017
@rlaverde
Copy link
Member Author

I'm not sure if this is a good change, directory and file names become really long and ugly .___.

I'm closing this, I think that's not to much harm to be asked two more questions, that will be ok to leave with the default value. Maybe we should add a comment in the Readme instead.

@rlaverde rlaverde closed this Jan 17, 2017
@rlaverde rlaverde removed their assignment Jan 17, 2017
@rlaverde rlaverde removed this from the v0.1 milestone Jan 17, 2017
@goanpeca goanpeca reopened this Jan 17, 2017
@goanpeca
Copy link
Member

goanpeca commented Jan 17, 2017

I'm not sure if this is a good change, directory and file names become really long and ugly

I don't understand how this is a problem? unless it does not work on windows or something is not enough reason

@rlaverde
Copy link
Member Author

I address your comments, anyway I dont like to have files named:

spyder-{{ cookiecutter.plugin_name.lower().replace(' ', '-') }}/spyder_{{ cookiecutter.plugin_name.lower().replace(' ', '_') }}}/widgets/{{ cookiecutter.plugin_name.lower().replace(' ', '') }}gui.py

and when writing in the terminal with escaping will become:

spyder-\{\{\ cookiecutter.plugin_name.lower\(\).replace\(\'\ \'\,\ \'-\'\)\ \}\}/spyder_\{\{\ cookiecutter.plugin_name.lower\(\).replace\(\'\ \'\,\ \'_\'\)\ \}\}/widgets/\{\{\ cookiecutter.plugin_name.lower\(\).replace\(\'\ \'\,\ \'\'\)\ \}\}gui.py

I think this is so error prone 😕

@@ -43,11 +43,11 @@ dependencies:
test:
override:
# Check Python style
- $CMD_LINT_CIOCHECK spyder-{{ cookiecutter.plugin_name.lower().replace(' ', '_') }} -dt: # note the colon
- $CMD_LINT_CIOCHECK spyder-{{ cookiecutter.plugin_name.lower().replace(' ', '-') }} -dt: # note the colon
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong we are calling the module here

parallel: true

# Run Python tests
- $CMD_TEST_PYTEST spyder-{{ cookiecutter.plugin_name.lower().replace(' ', '_') }} --cov=spyder-{{ cookiecutter.plugin_name.lower().replace(' ', '_') }}: # note the colon parallel: true
- $CMD_TEST_PYTEST spyder-{{ cookiecutter.plugin_name.lower().replace(' ', '-') }} --cov=spyder-{{ cookiecutter.plugin_name.lower().replace(' ', '-') }}: # note the colon parallel: true

Copy link
Member

Choose a reason for hiding this comment

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

same here

@rlaverde
Copy link
Member Author

Closing, I don't like the approach of this PR, I think is better to continue asking for 3 variables.

Maybe this change upstream cookiecutter/cookiecutter#364 will make this a lot easier

@rlaverde rlaverde closed this Sep 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants