Skip to content

Add more plugins and refactor infrastructure#29

Merged
pllim merged 10 commits intoastropy:masterfrom
astrofrog:infrastructure-and-additions
Jan 13, 2020
Merged

Add more plugins and refactor infrastructure#29
pllim merged 10 commits intoastropy:masterfrom
astrofrog:infrastructure-and-additions

Conversation

@astrofrog
Copy link
Member

@astrofrog astrofrog commented Jan 9, 2020

This PR:

…e, pytest-mpl and pytest-cov as dependencies, and updated README.
@astrofrog astrofrog requested review from bsipocz, eteq and pllim January 9, 2020 11:17
@Cadair
Copy link
Member

Cadair commented Jan 9, 2020

🚀

Copy link

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

🎉

Along with the comments below:

  • is pytest-astropy really still in alpha? What's left to implement?
  • could add the Programming Language :: Python :: 3 :: Only classifier
  • probably a good idea to have a minimum version of pytest-cov,
    as much for the conda resolver as for general hygiene.

setup.cfg Outdated
Comment on lines +41 to +42
# Bump hypothesis to >=5.0 in early Jan 2020
hypothesis>=4.53
Copy link

Choose a reason for hiding this comment

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

I think there's a decent argument for >=5.1 for timezone support and generally avoiding possible deprecated behaviour 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I've bumped this - thanks!

setup.cfg Outdated
license = BSD
description = Meta-package containing dependencies for testing
long_description = file: README.rst
keywords = pytest, py.test, remotedata, openfiles, doctestplus
Copy link

Choose a reason for hiding this comment

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

Pytest maintainer 🎩: the py.test alias is discouraged.

Hypothesis 🎩: could add hypothesis, property-based testing

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

@astrofrog
Copy link
Member Author

@Zac-HD - thanks for the review! I think I've addressed all your comments :)

Copy link
Contributor

@saimn saimn left a comment

Choose a reason for hiding this comment

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

Small comments otherwise looks good!

CHANGES.rst Outdated
==================

- Added ``pytest-mpl``, ``pytest-filter-subpackage`` and ``pytest-cov``
as dependencies. [#29]
Copy link
Contributor

Choose a reason for hiding this comment

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

Changelog should be expanded with things like hypothesis and Python 2.7 and 3.5 support.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

setup.cfg Outdated
@@ -0,0 +1,44 @@
[metadata]
name = pytest-astropy
url = https://astropy.org
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what's used for other astropy packages, but I find useful to have a more precise URL here, like the github url. Otherwise it is sometimes annoying to find the repository.

Copy link

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

That's very cool! For now I'll just link to the repo since for this specific package there aren't e.g. separate docs and so on, but I'll definitely be using this for other packages.

Copy link
Member

Choose a reason for hiding this comment

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

Oooo that is very convenient indeed, @Zac-HD , thanks! @astrofrog , any plans to use project_urls in your APE 17 PR over at astropy?

Copy link
Member Author

Choose a reason for hiding this comment

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

@pllim - yes I can try and do that

from setuptools import setup

# Setuptools 30.3.0 or later is needed for setup.cfg options to be used
if LooseVersion(setuptools.__version__) < LooseVersion('30.3.0'):
Copy link
Member

Choose a reason for hiding this comment

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

Why not put this in pyproject.toml?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is in case people call setup.py directly

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Okie dokie then. Merging.

Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Just a minor question, otherwise LGTM. Thanks!

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.

5 participants