-
-
Notifications
You must be signed in to change notification settings - Fork 63
Use pytest-astropy-header for test header #436
Conversation
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.
superficial review, we might need to go the other way around, require pytest-astropy-header, but not pytest-astropy
@@ -15,7 +15,7 @@ env: | |||
- FOLDERNAME='packagename' | |||
- PYTHON_VERSION=3.7 | |||
- CONDA_DEPENDENCIES='cython astropy sphinx' | |||
- PIP_DEPENDENCIES='cookiecutter gitpython pytest-astropy sphinx-astropy' | |||
- PIP_DEPENDENCIES='cookiecutter gitpython pytest-astropy pytest-astropy-header sphinx-astropy' |
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.
pytest-astropy should contain pytest-astropy-header
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 added it back in. For some reason, removing it caused Travis to fail for all but the last job. 🤷♀️
@@ -11,6 +11,7 @@ url = {{ cookiecutter.project_url }} | |||
edit_on_github = {{ cookiecutter.edit_on_github_extension }} | |||
github_project = {{ cookiecutter.github_project }} | |||
python_requires = ">={{ cookiecutter.minimum_python_version }}" | |||
tests_require = pytest; pytest-astropy; pytest-astropy-header |
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.
should pytest-astropy be a strict dependency?
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.
It will be with this patch because I removed the if ... else ...
check, which I think is more complicated than it's worth.
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.
yeah, but I think using pytest-astropy-header should just work for everything, isn't it (at least that was the reason I was pushing for having it as a separate plugin rather than shuffled into pytest-astropy)
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.
Correct. Theoretically it would work even if you have older astropy version, but that also makes it a compulsory dependency for tests. Or did I misunderstand your question?
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.
no I think we're on the same page. But then we don't need to list pytest-astropy here, right?
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.
Oh yeah...
When I next have chance, I have to see why the tests failed with |
The failure starts appearing after I removed |
Superseded by #438 |
Address #435 for new package being created off the template. Does not fix the problem of existing packages that used the older template.
TODO: