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

Readme fix #8

Closed
wants to merge 8 commits into from
Closed

Conversation

kritisingh1
Copy link

I have tried to come up with the solutions of the problems I faced while setting up the project.

  1. Since we are using Python 3.x therefore we need to use pip3 to install the required version of Django (here, 2.0.2) or whole of the requirements.txt file.
  2. The virtual environment folder and the Pipfile were not listed in the .gitignore and were consequently being tracked by git. So I have added them in the .gitignore file and also added the required file name in the setup instruction.
  3. Other two instructions modified are python3 manage.py migrate from python manage.py migrate and python3 manage.py runserver from python manage.py runserver as we are using python3 so the initial command is likely to use a lower version of python.

Copy link
Member

@gedankenstuecke gedankenstuecke left a comment

Choose a reason for hiding this comment

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

tl;dr: LGTM (looks good to me) 👍

the longer version: Thanks for those changes. I have to admit that I hadn't thought much about the correct specification of Python versions so far, largely because many distributions/package manager make sure that python points to your preferred python version. But it turns out there is a PEP for this and the changes proposed here adhere to PEP 394 as is now!?

@madprime you're the better Python programmer, do you agree with my interpretation?

@madprime
Copy link
Member

This looks great, and I'm not sure I'm a better Python programmer! I've only just learned that pipenv is the official workflow recommendation now.

So... I think the README fix should instead change this whole project to use pipenv and Pipfile instead of requirements.txt. pipenv is expected to be installed globally, not in local requirements.

(Sorry for the extra work! But I think this is ideal...)

This probably includes the following changes...

  1. Update the code to replace requirements.txt with Pipfile
  2. instruct people to have pipenv installed globally
  3. run pipenv --three install -r requirements.txt to create a virtual environment and install requirements
  4. run pipenv shell to activate it

This would make specifying python3 vs python redundant but harmless (it's fine to keep it).
And if I understand how it all works, the .gitignore should have Pipfile.lock added, but not the current additions. (The resulting virtual environment files will be stored away elsewhere, automatically.)

There may be other additions needed for instructions that become obvious as one goes through setup with a pipenv workflow. :)

.gitignore Outdated
my_project

#Pipfile
Pipfile
Copy link
Member

Choose a reason for hiding this comment

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

per my comment, rather than this being ignored, I think requirements.txt should be replaced with Pipfile

Copy link
Author

Choose a reason for hiding this comment

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

So should I just update the file name of requirements.txt as Pipfile ?

Copy link
Member

Choose a reason for hiding this comment

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

No, you should keep the Pipfile as created by pipenv --three install -r requirements.txt and commit to the repository too, along with Pipfile.lock (and remove both from the .gitignore)

Copy link
Author

@kritisingh1 kritisingh1 Feb 16, 2018

Choose a reason for hiding this comment

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

However the requirements.txt contains one additional dependency other than those in the generated Pipfile named six==1.11.0. How to tackle this? @madprime

Copy link
Member

Choose a reason for hiding this comment

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

Can you have a look into your Pipfile.lock? six should turn up there 😃

.gitignore Outdated
@@ -30,3 +31,9 @@ db.sqlite3

# dotenv (environment variables, used for secret/local stuff)
.env

#Name of the virtual environment
my_project
Copy link
Member

Choose a reason for hiding this comment

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

I believe pipenv automatically creates a virtual environment elsewhere (by default, in a user's .local directory) – let's switch to that, and then I think this becomes unneeded.

Copy link
Author

@kritisingh1 kritisingh1 Feb 16, 2018

Choose a reason for hiding this comment

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

Yeah it created a virtual environment like this in my case on running pipenv install requests :
Virtualenv location: /home/kriti/.local/share/virtualenvs/oh-proj-management-Yei1TVXn
I think Pipfile was also created by the same command.

Copy link
Member

Choose a reason for hiding this comment

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

Great! Sounds like you can remove this from the .gitignore, then. (If you make a new commit to this branch, your PR automatically updates.)

@gedankenstuecke
Copy link
Member

This looks great, and I'm not sure I'm a better Python programmer! I've only just learned that pipenv is the official workflow recommendation now.

Yeah, I didn't know that before either :D

So... I think the README fix should instead change this whole project to use pipenv and Pipfile instead of requirements.txt. pipenv is expected to be installed globally, not in local requirements.

Yes, that sounds good to me. I personally don't have a strong preference for any of these solutions and I guess in the end they all do the same thing if one uses them to create virtual environments.

And if I understand how it all works, the .gitignore should have Pipfile.lock added, but not the current additions. (The resulting virtual environment files will be stored away elsewhere, automatically.)

Nope, the Pipfile.lock should be committed too. It's the pipenv equivalent of Ruby's bundler package that creates Gemfile.lock files.

Committing these lock files too has the benefit that if you run install it will use the *.lock to install all the dependencies with the versions specified in the *.lock instead of randomly taking versions that are newer than the specified ones. This way you can easily guarantee that your production and development environment are running the same versions. If you then want to upgrade a package/all packages you can run pipenv update $package or pipenv update to update the packages in the *.lock file. Does that make sense? (also see the recommendation at pypa/pipenv#598)

@madprime
Copy link
Member

I think Pipfile specifies versions… and it seems Pipefile.lock is in pipenv's own .gitignore?https://github.com/pypa/pipenv/blob/master/.gitignore

@gedankenstuecke
Copy link
Member

In our current case that's true as we specify exact versions in our requirements.txt already. But in principle Pipfiles don't have to include any version, c.f. https://docs.pipenv.org/basics/

Indeed, the pipenv documentation urges you to not hard-code the version inside the Pipfile but rather use the Pipfile.lock for pinning down the exact versions. This seems to be to be one of the big benefits of the whole pipenv workflow. 😄

@gedankenstuecke
Copy link
Member

@madprime
Copy link
Member

Ah, ok, yes, keep it out of gitignore then. From those docs, this is relevant to webapp dev and deploy: You can use pipenv lock to compile your dependencies on your development environment and deploy the compiled Pipfile.lock to all of your production environments for reproducible builds.

@gedankenstuecke
Copy link
Member

Yep, the alternative would be to always specify the exact versions in the Pipfile but this has the drawback that you can't easily use pipenv update $package to update single packages (e.g. if you notice that one has a security vulnerability)

Copy link
Member

@madprime madprime left a comment

Choose a reason for hiding this comment

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

I think this looks good now, the only comment would be that I think we prefer having a Pipfile.lock. While I've merged @rosygupta's PR instead, please do reference this as a contribution, that work was certainly inspired by this! :)

@madprime
Copy link
Member

Thanks so much for this! PR #9 ended up being merged instead.

@madprime madprime closed this Feb 17, 2018
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