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

update requirements #308

Merged
merged 5 commits into from
Mar 17, 2022
Merged

update requirements #308

merged 5 commits into from
Mar 17, 2022

Conversation

olaurino
Copy link
Contributor

@olaurino olaurino commented Mar 11, 2022

When I started working with the repo I couldn't install all the dependencies right away and I had to figure out what I needed for testing, so I thought I'd create a little PR with the changes I made.

  • add pillow to the list of extra requirements. However note that one of the tests fails without pillow. Should the test be skipped if pillow is not installed?
  • add requests-mock to the list of test dependencies.

Also remove a left over pip-requirements file and fix #310

  • Review the README
  • Review the installation docs

@codecov
Copy link

codecov bot commented Mar 11, 2022

Codecov Report

Merging #308 (b145874) into main (7a0b7da) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #308   +/-   ##
=======================================
  Coverage   75.49%   75.49%           
=======================================
  Files          44       44           
  Lines        5125     5125           
=======================================
  Hits         3869     3869           
  Misses       1256     1256           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@bsipocz
Copy link
Member

bsipocz commented Mar 11, 2022

The PR (#290) that switched to the new build system missed to update the docs, and I would suggest to update the install the dev version session and to add this new test related one to rely solely on the info in setup.cfg, as in my experience adding new requirements files is a chance for getting them off sync over time. I would also suggest to remove the pip_requirements file, I think it's accidentally left in there as it's not being used anywhere.

So, the syntax to get all you need for testing:

pip install -e .[test]

For building the docs:

pip install -e .[docs]

And getting all optional dependencies:

pip install -e .[all]

These should of course work with non executable dev versions when changing -e . to pyvo. If there are any dependencies that are still missing after running these commands, those should be added to the appropriate section of [options.extras_require].

@olaurino
Copy link
Contributor Author

@bsipocz thanks. I'll try again with the info you provided. I was indeed confused by all the different files.

@bsipocz
Copy link
Member

bsipocz commented Mar 11, 2022

I think the README needs updating, though. It mentions running setup.py directly.

yes, that was a mistake in the review of the PR that we didn't spot that the documentation was missed out from the updates.

@olaurino
Copy link
Contributor Author

@bsipocz well this was fun. I hadn't had the luxury to work with a python only system in a while. Only conda for a long time! The sphinx installation documentation seems very basic. Is it enough to replace python setup.py install with pip install and maybe add a line or two on running the tests?

@bsipocz
Copy link
Member

bsipocz commented Mar 11, 2022

The sphinx installation documentation seems very basic. Is it enough to replace python setup.py install with pip install and maybe add a line or two on running the tests?

I think it's enough, but it's really you who can tell what would have been enough info to help you get set up.

Copy link
Contributor

@andamian andamian left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@andamian andamian merged commit b39eef7 into astropy:main Mar 17, 2022
@andamian
Copy link
Contributor

Thanks for the contribution @olaurino

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework tox to rely on setup.cfg to install dependencies
3 participants