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

docs(readme): update installation instructions #713

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

corenting
Copy link
Contributor

@corenting corenting commented Aug 22, 2023

  • Changed the documentation installation method with pip to use --user to prevent
    installing system-wide. It's not recommended to install system wide and may not work anyway on OS where the system Python is marked as externally managed (see PEP 668. It's the case for Debian 12 for example.
  • Documented the fact that the tool can also be installed with pipx.

@corenting corenting added the type:doc Documentation improvements label Aug 22, 2023
@corenting corenting requested a review from agateau-gg August 22, 2023 14:07
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2023

Codecov Report

Merging #713 (a48281f) into main (b084f89) will decrease coverage by 0.13%.
Report is 20 commits behind head on main.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #713      +/-   ##
==========================================
- Coverage   92.33%   92.20%   -0.13%     
==========================================
  Files         135      135              
  Lines        5831     5839       +8     
==========================================
  Hits         5384     5384              
- Misses        447      455       +8     
Flag Coverage Δ
unittests 92.20% <ø> (-0.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 89 files with indirect coverage changes

README.md Outdated
Alternatively, you can use [pipx](https://pypa.github.io/pipx/) to install and run the application in an isolated environment:

```shell
$ pipx install ggshield
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would go even further and recommend pipx first. It's easier to maintain and works in externally managed environments where pip install does not (even with --user, I was curious and just checked)

@@ -76,7 +82,7 @@ The package should run on MacOS, Linux and Windows.
To update `ggshield` you can add the option `-U/--upgrade` to the pip install command.

```shell
$ pip install -U ggshield
$ pip install --user -U ggshield
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we explain how to install with pipx then we must also explain how to upgrade with it here.

@@ -0,0 +1,5 @@
### Changed
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this needs to be in the changelog: the target audience for release notes are people who already have the app installed, they don't really care about how to do it :)

Copy link
Collaborator

@agateau-gg agateau-gg left a comment

Choose a reason for hiding this comment

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

minor nitpicks

README.md Outdated

### Installing

The recommended way to install `ggshield`` is to use [pipx](https://pypa.github.io/pipx/), which will install it an isolated environment:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The recommended way to install `ggshield`` is to use [pipx](https://pypa.github.io/pipx/), which will install it an isolated environment:
The recommended way to install `ggshield` is to use [pipx](https://pypa.github.io/pipx/), which will install it an isolated environment:

README.md Outdated
$ pipx install ggshield
```

Alternatively, you can install with pip as an user package. This will not work if your Python installation is declared as externally managed (for example if using the system Python on some operating systems):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Alternatively, you can install with pip as an user package. This will not work if your Python installation is declared as externally managed (for example if using the system Python on some operating systems):
Alternatively, you can install with pip as a user package. This will not work if your Python installation is declared as externally managed (for example when using the system Python on operating systems like Debian 12):

@corenting corenting requested a review from agateau-gg August 23, 2023 08:23
Copy link
Collaborator

@agateau-gg agateau-gg left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

I let you squash the fixups before I merge.

@corenting corenting force-pushed the cgarcia/-/update-readme branch from a48281f to 1dfd803 Compare August 23, 2023 09:34
@corenting
Copy link
Contributor Author

Looks good, thanks!

I let you squash the fixups before I merge.

Thanks for the review! I squashed the PR & rebased on the latest main.

@agateau-gg agateau-gg enabled auto-merge August 23, 2023 09:38
@agateau-gg agateau-gg merged commit 136eb5e into main Aug 23, 2023
@agateau-gg agateau-gg deleted the cgarcia/-/update-readme branch August 23, 2023 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog type:doc Documentation improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants