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.txt #58

Merged
merged 7 commits into from
Jul 4, 2020
Merged

Update requirements.txt #58

merged 7 commits into from
Jul 4, 2020

Conversation

gstein
Copy link
Contributor

@gstein gstein commented Jul 4, 2020

The "graphviz" Python module is required. Clarify that within the requirements.

(the graphviz/dot commands are needed at the OS level, but that can't be fixed here)

17o2 and others added 6 commits June 28, 2020 15:00
The "graphviz" Python module is required. Clarify that within the requirements.

(the graphviz/dot commands are needed at the OS level, but that can't be fixed here)
@17o2
Copy link
Collaborator

17o2 commented Jul 4, 2020

What's the difference between declaring this in requirements.txt as opposed to setup.py (see linked line)?

Since I'm not familiar with creating Python packages myself, maybe @n42 can be of help as the author of the refactoring?

If we add it to requirements.txt, pyyaml should also be listed.

@gstein
Copy link
Contributor Author

gstein commented Jul 4, 2020

That's quite a fair question, and I see that pyyaml is in setup.py (which could/should also be moved!) ... by moving it to requirements.txt, tools can consume the set of needed modules (whereas, they cannot discover the set from setup.py).

For example:
$ pip install -r requirements.txt

@gstein
Copy link
Contributor Author

gstein commented Jul 4, 2020

(honestly, I didn't even stop to look into setup.py ... I always go to requirements.txt first :p )

@n42
Copy link
Contributor

n42 commented Jul 4, 2020

A difference is iirc that requirements.txt is mainly useful in that it's easily human-readable and can be used for explicitly installing only the dependencies, which might be useful for using packages without installing them first. Pip/setuptools will use what's listed in setup.py as required dependencies and install them automatically if you use that to install the package.

@n42
Copy link
Contributor

n42 commented Jul 4, 2020

It doesn't hurt anything to use both methods, but I'd suggest adding the pyyaml dep in the reqs file as well if you want to use that.

@17o2
Copy link
Collaborator

17o2 commented Jul 4, 2020

So it's fine to have in both places? I'll just add two lines into requirements.txt file and leave setup.py untouched. Thanks.

@n42
Copy link
Contributor

n42 commented Jul 4, 2020

Yeah, perfectly fine. There should be a way to have setup.py parse requirements.txt instead of having multiple places to define them as well, but I seem to recall something about pip version fuckery wrt that. I'll look into it when I'm at an actual computer.

@17o2
Copy link
Collaborator

17o2 commented Jul 4, 2020

Cheers! I'll wait until then, I don't see this as a super urgent issue.

shift the pyyaml dependency out of setup.py to requirements.txt
@gstein
Copy link
Contributor Author

gstein commented Jul 4, 2020

Agree: not urgent. Per discussion, I've included pyyaml into the PR.

@17o2 17o2 changed the base branch from master to dev July 4, 2020 14:51
@17o2 17o2 merged commit 8b067e5 into wireviz:dev Jul 4, 2020
@17o2
Copy link
Collaborator

17o2 commented Jul 4, 2020

Merged and closed for now. If we want to update setup.py to parse requirements.txt in the future, we can open a new PR. In the meantime, if more requirements appear, they'll be added to both files.

@gstein gstein deleted the patch-1 branch July 4, 2020 21:48
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