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

Remove direct installation instructions, link to docs #2642

Merged
merged 1 commit into from Oct 4, 2019

Conversation

@slhck
Copy link
Contributor

commented Oct 1, 2019

Change default installation instructions to remove use of "sudo", as it is highly discouraged.

References:

Other very popular packages also refraining from using "sudo" in their documentation:


Edit: PR changed according to feedback.

@Uinden

This comment has been minimized.

Copy link

commented Oct 2, 2019

Isn't it pip install streamlink --user?
Without --user it will fail. At least on my installation.
Like error: could not create '/usr/lib/python3.7/site-packages/%package%': Permission denied

@slhck

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2019

It depends!™️

If you're using a user-environment Python like from pyenv or Homebrew, or Pipenv, or a Virtualenv, then you don't need --user. On a “standard” Linux with system Python, you need --user.

If there's a danger that people might just do sudo if they see a “permission denied” error, then perhaps we should add --user. It doesn't hurt.

@bastimeyer

This comment has been minimized.

Copy link
Member

commented Oct 2, 2019

The install instructions on the repo's README are a remnant of the Livestreamer fork, I believe. Streamlink's documentation has a dedicated install page, where the pip install --user situation is explained, although kind of mixed with installing it from git, see here:
https://streamlink.github.io/install.html#source-code

Not sure if everyone else agrees with me here, but I think it would be better to remove the install commands from the README as a whole and just refer to the install documentation instead. Ideally, this should have been done when the the install page and Github release template got rewritten earlier this year, but I forgot about it.

I won't close any PRs this month due to Hacktoberfest, but if we agree on removing the commands, I would be happy if you could change this PR real quick. If not, then just removing the sudo part is also fine.

@codecov

This comment has been minimized.

Copy link

commented Oct 2, 2019

Codecov Report

Merging #2642 into master will increase coverage by 0.26%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2642      +/-   ##
==========================================
+ Coverage   52.45%   52.72%   +0.26%     
==========================================
  Files         242      242              
  Lines       15115    15115              
==========================================
+ Hits         7929     7969      +40     
+ Misses       7186     7146      -40
@gravyboat

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

@bastimeyer I agree. I think a link to the actual install docs would be better, then we don't have to maintain them in two locations.

@slhck slhck force-pushed the slhck:patch-1 branch from 16a8d98 to 7d70cf6 Oct 3, 2019
@slhck

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

Thanks for your feedback everyone. I've changed the README to just link to the documentation for installation instructions. PR title changed accordingly.

@slhck slhck changed the title do not suggest "sudo" for pip Remove direct installation instructions, link to docs Oct 3, 2019
@slhck slhck force-pushed the slhck:patch-1 branch from 7d70cf6 to f95f5be Oct 3, 2019
Copy link
Member

left a comment

Looks good, let's confirm with others as well.

Copy link
Member

left a comment

This is better, thanks!

Just noticed that the link anchors of the list below are wrong/outdated. That's not an issue of this PR though. If you want, @slhck, you could fix this as well, if not, then we can merge this.

Remove the direct installation instruction containing "sudo" as well as manual
installation, and link to the documentation instead.

Fix links to documentation pages.
@slhck slhck force-pushed the slhck:patch-1 branch from f95f5be to 31ce547 Oct 4, 2019
@slhck

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

You're right. I fixed those links, removed the explicit mention of Windows portable (since it's linked from the first link anyways), added macOS and the source code installation.

@gravyboat

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

Looks good, thanks @slhck!

@gravyboat gravyboat merged commit d78efc4 into streamlink:master Oct 4, 2019
3 checks passed
3 checks passed
codecov/project 52.72% (target 30%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.