-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Removed all references to setup.py test fixes #500
#544
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
base: main
Are you sure you want to change the base?
Removed all references to setup.py test fixes #500
#544
Conversation
jamesmyatt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing #500 for me. I won't request these changes since I'm just not a maintainer of this project; just a community member.
| with bake_in_temp_dir(cookies, extra_context={'full_name': 'name "quote" name'}) as result: | ||
| assert result.project.isdir() | ||
| run_inside_dir('python setup.py test', str(result.project)) == 0 | ||
| run_inside_dir('python setup.py --help', str(result.project)) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't do what the test name says
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right... When I saw the docstring I rationalized that the only reason it was running the tests was to validate that setup.py was formatted correctly. Do you agree/does this accomplish the same objective with less overhead? If so I'll rename it, if not I'll change it to run tests.
| with bake_in_temp_dir(cookies, extra_context={'full_name': "O'connor"}) as result: | ||
| assert result.project.isdir() | ||
| run_inside_dir('python setup.py test', str(result.project)) == 0 | ||
| run_inside_dir('python setup.py --help', str(result.project)) == 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't do what the test name says
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see previous comments
| [flake8] | ||
| exclude = docs | ||
|
|
||
| [aliases] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove this section if it's empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left this in to demonstrate how it's done, but I can certainly remove it
| {% if cookiecutter.use_pytest == 'y' -%} | ||
| commands = | ||
| pip install -U pip | ||
| pytest --basetemp={envtmpdir} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above, the command was pytest ./tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this down because requirements_dev.txt needs to be installed irrespective of which test suite is in use.
Edit: I think I'm confused... does L28 need to be changed? I didn't understand the intent of this line but left it unchanged since it was not impacted by the change
…ckage into bugfix/500-setuppy-test-removal
…st references from setup.py
Note that this was done as a separate bugfix and does not include the linting and test fixes in my prior PR (#543). I can resubmit with both integrated if needed