Skip to content

Conversation

@Bost
Copy link
Contributor

@Bost Bost commented Mar 26, 2020

No description provided.

@gribok
Copy link
Contributor

gribok commented Mar 26, 2020

Can you please add pipenv command to install dev requirements as well?
See #204

@Bost
Copy link
Contributor Author

Bost commented Mar 26, 2020

Added.

README.md Outdated
* `make test`
```bash
pipenv shell
pipenv install --dev
Copy link
Collaborator

@Kilo59 Kilo59 Mar 26, 2020

Choose a reason for hiding this comment

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

This should probably be pipenv sync --dev.
install will re-resolve all the dependencies if packages have been updated, potentially leading to a problem.
sync installs whatever is in Pipfile.lock(which is what we want) .
https://pipenv.pypa.io/en/latest/advanced/#using-pipenv-for-deployments

README.md Outdated

Now, make sure you're running python 3.8 and that your `pipenv` actually uses it! It may use packages from `$HOME/.local/lib/python3.7` even when started with `pipenv --python 3.8`. To combat this, (re)install it using:
```
/path/to/python3.8 -m pip install pipenv
Copy link
Collaborator

@Kilo59 Kilo59 Mar 26, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then you need to install pipx in the first place, which is yet another thing which can go wrong. So when having the choice - to get X done:

  1. you need to have Y.
  2. just use tools you already have.

I prefer the 2., unless given sufficiently strong arguments for 1.

Anyway I'm not much of a python developer, and I don't really want to become one, so it's up to you - change it the way you like. That applies to both or your remarks.

Copy link
Collaborator

@Kilo59 Kilo59 Mar 26, 2020

Choose a reason for hiding this comment

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

Arguments about the best way to install pipenv aside...
What you've written here is factually incorrect.
You don't need to re-install pipenv in order to have it create a 3.8 virtual environment.
pipenv installed via 3.6 can still create and use a 3.8 environment as long as 3.8 is installed and on the PATH
https://pipenv.readthedocs.io/en/latest/basics/#specifying-versions-of-python

You don't need to include detailed instructions about brew or pipx installations.
Something simple with links to more detailed instructions should be sufficient.


  1. Make sure you have python3.8 installed and on your PATH.
  2. Install the pipenv dependency manager

The test client uses the requests library for making http requests.
"""
return TestClient(APP)

Copy link
Collaborator

@Kilo59 Kilo59 Mar 26, 2020

Choose a reason for hiding this comment

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

We now require that the code be formatted with the black code formatter.
That's why the build is failing.
You can do pipenv run fmt if your branch is up to date.
or black . -l 120 directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ups, that was a mistake. I didn't want to change the tests/conftest.py. Sorry

@ExpDev07 ExpDev07 requested a review from Kilo59 March 26, 2020 19:41
Copy link
Collaborator

@Kilo59 Kilo59 left a comment

Choose a reason for hiding this comment

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

Instructions contain misleading information about pipenv usage.

README.md Outdated

Now, make sure you're running python 3.8 and that your `pipenv` actually uses it! It may use packages from `$HOME/.local/lib/python3.7` even when started with `pipenv --python 3.8`. To combat this, (re)install it using:
```
/path/to/python3.8 -m pip install pipenv
Copy link
Collaborator

@Kilo59 Kilo59 Mar 26, 2020

Choose a reason for hiding this comment

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

Arguments about the best way to install pipenv aside...
What you've written here is factually incorrect.
You don't need to re-install pipenv in order to have it create a 3.8 virtual environment.
pipenv installed via 3.6 can still create and use a 3.8 environment as long as 3.8 is installed and on the PATH
https://pipenv.readthedocs.io/en/latest/basics/#specifying-versions-of-python

You don't need to include detailed instructions about brew or pipx installations.
Something simple with links to more detailed instructions should be sufficient.


  1. Make sure you have python3.8 installed and on your PATH.
  2. Install the pipenv dependency manager

@Kilo59 Kilo59 added the documentation Improvements or additions to documentation label Mar 26, 2020
@Bost
Copy link
Contributor Author

Bost commented Mar 26, 2020

What you've written here is factually incorrect.

Whatever. Correct me as you please. Write there what you want and need.
As I said, I'm really not interested it these things, the same way e.g. I'm not interested in manually changing gears of my car. All I want is to move forward, backward, left or right, slower or faster. I'm not interested in pushing or pulling any levers, strings, buttons pedals whatever. In fact I'm not even interested in cars. All I want to get from A to B. In this case get productive again, get my dev environment running. Thanks.

@Kilo59 Kilo59 requested a review from ExpDev07 March 27, 2020 00:14
@Bost Bost mentioned this pull request Mar 27, 2020
@ExpDev07 ExpDev07 merged commit b12ba72 into ExpDev07:master Mar 27, 2020
@ExpDev07 ExpDev07 linked an issue Mar 27, 2020 that may be closed by this pull request
@ExpDev07 ExpDev07 removed a link to an issue Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants