Re: [PATCH v4 08/24] python: Add pipenv support

[Cleber Rosa]

On Wed, Feb 17, 2021 at 12:28:22PM -0500, John Snow wrote:
> On 2/16/21 10:02 PM, Cleber Rosa wrote:
> > On Tue, Feb 16, 2021 at 09:59:47PM -0500, Cleber Rosa wrote:
> > > On Thu, Feb 11, 2021 at 01:58:40PM -0500, John Snow wrote:
> > > > pipenv is a tool used for managing virtual environments with pinned,
> > > > explicit dependencies. It is used for precisely recreating python
> > > > virtual environments.
> > > > 
> > > > pipenv uses two files to do this:
> > > > 
> > > > (1) Pipfile, which is similar in purpose and scope to what setup.py
> > > > lists. It specifies the requisite minimum to get a functional
> > > > environment for using this package.
> > > > 
> > > > (2) Pipfile.lock, which is similar in purpose to `pip freeze >
> > > > requirements.txt`. It specifies a canonical virtual environment used for
> > > > deployment or testing. This ensures that all users have repeatable
> > > > results.
> > > > 
> > > > The primary benefit of using this tool is to ensure repeatable CI
> > > > results with a known set of packages. Although I endeavor to support as
> > > > many versions as I can, the fluid nature of the Python toolchain often
> > > > means tailoring code for fairly specific versions.
> > > > 
> > > > Note that pipenv is *not* required to install or use this module; this 
> > > > is
> > > > purely for the sake of repeatable testing by CI or developers.
> > > > 
> > > > Here, a "blank" pipfile is added with no dependencies, but specifies
> > > > Python 3.6 for the virtual environment.
> > > > 
> > > > Pipfile will specify our version minimums, while Pipfile.lock specifies
> > > > an exact loudout of packages that were known to operate correctly. This
> > > 
> > > Layout? Loadout?
> > > 
> > > > latter file provides the real value for easy setup of container images
> > > > and CI environments.
> > > > 
> > > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > > ---
> > > >   python/Pipfile | 11 +++++++++++
> > > >   1 file changed, 11 insertions(+)
> > > >   create mode 100644 python/Pipfile
> > > > 
> > > 
> > > Other than that,
> > > 
> > > Reviewed-by: Cleber Rosa <crosa@redhat.com>
> > 
> > Actually, just one suggestion: bump the position of this patch twice.
> > It makes it easier to understand its purpose if it is placed right
> > before the "python: add pylint to pipenv" patch.
> > 
> > Cheers,
> > - Cleber.
> > 
> 
> The way the series is laid out is:
> 
> 01-02: pre-requisite fixes
> 03-07: Create the package, readmes, etc.
> 08:    Pipenv support
> 09-11: Pylint
> 12-13: flake8
> 14-15: mypy
> 16-17: isort
> 18-20: Testing and pre-requisites
> 21-23: Polish
> 24: CI support
> 
> Moving the pipenv patch to just before the final pylint patch works OK, but
> breaks up the pylint section. Should I still do it?
>

OK, now with that approach of groupping in min, it sounds reasonable.
My previous point was that pipenv is not needed until right before #10,
but that's just nitpicking and almost bikeshedding (I won't admit it
easily).

> --js
> 
> 
> (Hm, by this layout, I should probably actually move the pylint fix in #01
> down to appear after the pipenv patch. I could also move the flake8 fixes in
> #21 up to be near the other flake8 patches.)

Sounds more consistent.

Cheers,
- Cleber.

signature.asc
Description: PGP signature