Skip to content

WIP: Convert project to use pipenv #38

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

Merged
merged 1 commit into from
Nov 5, 2018

Conversation

okraits
Copy link
Member

@okraits okraits commented Sep 12, 2018

This PR has the goal of converting this project to use pipenv (https://pipenv.readthedocs.io/) for its development and deployment. It can serve as an example and evaluation of the conversion process.

For simplicity, I removed the method to parse the requirements.txt file from setup.py and inserted the dependencies directly, that's how other projects do it, too - https://github.com/requests/httpbin for example.

It also makes sense to keep both dependency lists - see https://pipenv.readthedocs.io/en/latest/advanced/#pipfile-vs-setuppy.

Also, I didn't find a possibility to parse the Pipfile so far (pipenv is the reference implementation - see https://github.com/pypa/pipfile), but we can add that later.

Todos:

  • fix travis build
  • optional: in setup.py, read dependencies from Pipfile

Related issue in pipenv: pypa/pipenv#1263

@okraits okraits changed the title Convert project to use pipenv WIP: Convert project to use pipenv Sep 12, 2018
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

The only way that comes to my mind to solve the duplication without requiring pipenv to install the package (which would be bad as explained by them on that issue you linked), is to parse the Pipfile using a regular expression that splits in groups based on encoutering square brackets, and then processing each line in the [packages] further. Sounds a bit overkill, but if we manage to do this well we could send a patch to setuptools and get it included there.

We could split this task into multiple GCI tasks as well.

@okraits okraits force-pushed the master-pipenv branch 2 times, most recently from 6d95900 to 2942de1 Compare September 19, 2018 13:28
@okraits
Copy link
Member Author

okraits commented Sep 19, 2018

Using the method described in pypa/pipenv#1263 (comment) I modified the Pipfile so that when issuing pipenv install it installs the local openwisp-controller module (pulling in the dependencies from setup.py.

This means that the runtime dependencies are defined in the install_requires list in setup.py and the development/test requirements are defined in Pipfile.

It would also be possible to define another list for dev packages in setup.py but the way I'm using is more clean IMO because there's a clean separation between packaging (setup.py) and development/deployment (Pipfile).

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@okraits Cool!
The .travis.yml file needs to be updated to reflect these changes otherwise the build will fail. See also my other minor comment.

Pipfile Outdated
coverage = "*"
coveralls = "*"
isort = "*"
flake8 = "*"
Copy link
Member

Choose a reason for hiding this comment

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

final new line is missing here

Copy link
Member Author

@okraits okraits Sep 19, 2018

Choose a reason for hiding this comment

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

Yes, already noticed it in the diff. Fixed 😀

I need to fix the travis build tomorrow.

@okraits okraits force-pushed the master-pipenv branch 6 times, most recently from 8643ceb to 1a1fe11 Compare September 20, 2018 09:54
@okraits okraits force-pushed the master-pipenv branch 2 times, most recently from ddf631c to 0632c24 Compare October 30, 2018 14:29
@coveralls
Copy link

coveralls commented Oct 30, 2018

Coverage Status

Coverage decreased (-0.2%) to 99.374% when pulling 87c42ce on TDT-AG:master-pipenv into fba9b8f on openwisp:master.

@jerbob
Copy link
Contributor

jerbob commented Nov 1, 2018

I'd recommend adding pipenv tasks to replace the current flake8, isort and runtests.py commands. I'm hoping to implement this in a PR to the TDT-AG repo, so this should be part of the PR soon.

@nemesifier
Copy link
Member

@anonguy great 👍

@nemesifier nemesifier merged commit 87c42ce into openwisp:master Nov 5, 2018
nemesifier added a commit that referenced this pull request Nov 5, 2018
pandafy pushed a commit to pandafy/openwisp-controller that referenced this pull request Dec 18, 2024
[qa] Added a check for empty line at the end of a file openwisp#29
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.

4 participants