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

doc: python: refreshing virtualenv section for venv #77569

Merged
merged 1 commit into from Jan 21, 2020

Conversation

d-goldin
Copy link
Contributor

Motivation for this change

Updating section about imperative use of ad-hoc virtual-environments
for use of pythons built-in venv module. Also trying to make it a bit
friendlier to beginners by adding a bit more explanation to the code
snippet and some remarks about using python 2.7 + virtualenv
and making it a bit more usable out of the box by not trying to re-create
the virtual environment each time.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

LGTM

@FRidh
Copy link
Member

FRidh commented Jan 13, 2020

If this is a supposedly popular way of working, why don't we provide a hook for this?

@jonringer
Copy link
Contributor

what do you recommend this looking like?

  (python.pkgs.virtualenv.venvHook.withName "dev-venv")
  python.pkgs.virtualenv.venvHook # alias for .withName "venv"

@FRidh FRidh mentioned this pull request Jan 13, 2020
10 tasks
@FRidh
Copy link
Member

FRidh commented Jan 13, 2020

Please have a look at #77644.

@jonringer
Copy link
Contributor

I like the venvShellHook better in #77644

@d-goldin
Copy link
Contributor Author

@jonringer: I think the shellhook is a decent idea to improve out of the box ease of use, but think the doc could be improved a bit, so I will adjust this PR to mention the hook, but I also think it might make sense to leave the "manual way" for educational reasons, for example if people need to adjust it to old-school virtualenv or similar. Does that make sense?

@ofborg ofborg bot removed the 6.topic: python label Jan 14, 2020
@d-goldin
Copy link
Contributor Author

If this is a supposedly popular way of working, why don't we provide a hook for this?

Just to add why I wanted to minimally improve this example: While not overly idiomatic from nix perspective, I think this is a workflow that many new-comers are looking at when doing python. Likely because they have to deal with some projects / dependencies that are not packaged but also lack understanding yet of how to do it more idiomatically. I've observed questions about this a few times on IRC so decided to maybe add a few words to lower barrier to entry / for gradual transition.

@FRidh
Copy link
Member

FRidh commented Jan 14, 2020

I think the explanation is good and helpful. Please use python3 instead of python2 though.

@d-goldin
Copy link
Contributor Author

Oh, sorry, copy paste fail. Also forgot the pip install post hook. Will fix.

@jonringer
Copy link
Contributor

I wasn't trying to say this wasn't worthwhile, it's still an improvement to documentation which usually can always need some help.

I was just trying to express that venvShellHook is a lot less repetitious

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for updating docs :)

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

otherwise LGTM

doc/languages-frameworks/python.section.md Outdated Show resolved Hide resolved
Updating section about imperative use of ad-hoc virtual-environments for
use of pythons built-in `venv` module via venvShellHook.  Also trying to
make it a bit friendlier to beginners by adding a bit more explanation
to the code snippet and some remarks old-school virtualenv.

Adjusting for venvShellHook and adding manual example

Adding pip install and replacing python2 example with python3
@d-goldin
Copy link
Contributor Author

I don't have merge rights, so would be nice if someone else could do that.

@jonringer
Copy link
Contributor

sorry, forgot about this

@jonringer jonringer merged commit 25d0d2b into NixOS:master Jan 21, 2020
@d-goldin d-goldin deleted the python_doc_venv branch January 26, 2020 15:54
d-goldin added a commit to d-goldin/nixpkgs that referenced this pull request Feb 2, 2020
When updating the section to python 3 some places still
referred to pythonPackages and were overlooked.
Decided to switch it to be more similar to the first
example binding pythonPackages and clarified comments a
bit based on confusion I observed on IRC.

Related to NixOS#77569
jonringer pushed a commit that referenced this pull request Feb 2, 2020
When updating the section to python 3 some places still
referred to pythonPackages and were overlooked.
Decided to switch it to be more similar to the first
example binding pythonPackages and clarified comments a
bit based on confusion I observed on IRC.

Related to #77569
jpgu-epam pushed a commit to jpgu-epam/nixpkgs that referenced this pull request Feb 4, 2020
When updating the section to python 3 some places still
referred to pythonPackages and were overlooked.
Decided to switch it to be more similar to the first
example binding pythonPackages and clarified comments a
bit based on confusion I observed on IRC.

Related to NixOS#77569
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.

None yet

3 participants