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 installation (redux) #114

Merged
merged 10 commits into from
Aug 31, 2021

Conversation

cthoyt
Copy link
Member

@cthoyt cthoyt commented Aug 23, 2021

This is a re-do of #90, which got merged into #113 and created a bit of confusion. The original text read:

This PR updates the way requirements are installed. Notably, it removes the requirements.txt, Pipfile, and Pipfile.lock since those are typically only used for web applications and one-off scripts. There's the little caveat where this means some installation dependencies can get mixed up, but Python is provably bad at this, and it's a crapshoot anyway (especially if you expect someone to install your software).

The requirements for docs and tests get put in their own extras inside the setup.cfg. All installation is changed to use pip. Tox allows you to specify which extras you want (should be obvious from the diff)

@hrshdhgd
Copy link
Contributor

hrshdhgd commented Aug 26, 2021

Hey @cthoyt ! I downloaded this branch locally and ran tox. Everything went smoothly except an error in between:
ERROR: InvocationError for command ../sssom-py-update-installation/.tox/flake8/bin/flake8 sssom/ tests/ setup.py (exited with code 1)

and towards to the end: ERROR: flake8: commands failed

I have just started learning tox and it may be that there's something wrong with the flake8 installation on my side? I assumed flake8 would be automatically installed on my system while running tox. Would this be an accurate assumption?
cc: @matentzn

UPDATE: I just noticed:

[testenv:flake8]
skip_install = true

So flake8 is not installed when I run tox. It assumes that the installation was already done prior to running tox ?

@cthoyt
Copy link
Member Author

cthoyt commented Aug 26, 2021

@hrshdhgd no surprise there, just read what comes before and you will see that the flake8 command is failing because it has several suggestions for improving the code. This PR is not about code cleanup, so don't consider this at the moment.

skip_install says that the sssom-py doesn't need to be installed with pip to run this test, not that flake8 doesn't need to be installed. You can read more here https://tox.readthedocs.io/en/latest/config.html#conf-skip_install

setup.cfg Show resolved Hide resolved
@cthoyt
Copy link
Member Author

cthoyt commented Aug 31, 2021

@matentzn @hrshdhgd can you please define what the acceptance/merge criteria for this PR are? I'm worried that this will sit too long and become stale again

@matentzn matentzn merged commit 2429f64 into mapping-commons:master Aug 31, 2021
@matentzn
Copy link
Collaborator

@cthoyt I was just working on it.. :D Mind reading.

@cthoyt cthoyt deleted the update-installation branch August 31, 2021 10:19
@cthoyt
Copy link
Member Author

cthoyt commented Aug 31, 2021

@matentzn woohoo!!

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.

3 participants