Strict dependency on black prevents usage of the latest black

[alexey-vyskubov]

pipenv-setup depends on black==19.10b0, while the latest black version is 20.8b1. This prevents the usage of pipenv-setup in any project which requires the latest black version (downgrading black is not an option, because 19.10b0 complains about formatting of the files 20.8b1 is happy with).

[alexey-vyskubov]

Is the project dead?...

[jshwi]

@alexey-vyskubov I don't think there are any commits from Madoshakalaka since Jun 29, 2020, and the license copyright is for 2019.

Do you know if anyone has attempted to reached out to the author yet?

[bryant-finney]

I reached out to the author today. I don't know what their upcoming availability will look like, but I would like to rekindle this project if possible

[jshwi]

I reached out too. I did not want to fork or start a "pipenv-setup2" or anything if avoidable, especially since this already had a few open PRs. I'm happy to help out too if needed.

[bryant-finney]

I can help knock this out now 👌

This PR needs to pin a black version as a dev extra dependency (otherwise this project's development environment will have ambiguous formatting).

I'll try to push a commit later today for doing this; however, if you can get to this before me then feel free to do so!

[jshwi]

@bryant-finney I think #36 could possibly be merged with hassle

edit: Sorry, I might be confusing myself it looks like you've already reviewed it :)

[bryant-finney]

okay about to take another swing at this one 🙂

[jshwi]

@bryant-finney 100%

I think there was a communication somewhere in this repo outlining why this was a prod dependency, seems like it would be for good reason. Would you be able to fill me on the situation with this?

[bryant-finney]

@jshwi absolutely!

Related Discussions

• this discussion on package update, add missing dependency chardet #75
  • summary: we can't simply un-pin black, as that will break pipenv install pipenv-setup for projects that haven't set allow_prereleases = true
• this discussion on relax the hard dependencies a bit #78
  • summary: it appears that this behavior may have changed with the latest pipenv release (/cc @jkogler-cloudflight)

Proposed Solution

I'm trying out this concept in the draft #88 : drop both black and autopep8 from the minimal dependencies, instead creating black and autopep8 sets of extras.

This proposal should allow the following behaviors:

• pipenv install pipenv-setup does not produce black version conflicts with the dependent application
  • neither autopep8 nor black would be requirements; if neither of these formatters are installed in the dependent environment, the default behavior would simply not apply stylistic changes to setup.py
• pipenv install "pipenv-setup[black]" and pipenv install "pipenv-setup[autopep8]" could be used to install the respective formatter
  • we could leave these versions unpinned, noting the requirement that allow_prereleases = true must be set for projects using black

Potential Issues

• some projects might assume pipenv-setup provides the correct black version
  • I would argue that this assumption should not be made; pipenv-setup is not a formatting package, so black vs. autopep8 vs. YAPF etc. are out-of-scope for fulfilling this project's primary purpose
  • after poking around GitHub a bit, the it seems to me that the biggest risk of breaking changes comes to projects that define pre-commit hooks for pipenv-setup and black
    • I tried finding an example of this, but I wasn't: each project I sampled either did not include a pre-commit hook for black, or it was managed independently

How does this proposal sound? Any suggestions? 🙂

[jshwi]

@bryant-finney Thanks heaps for the rundown, much appreciated

With what you've provided, this is my 2 cents

#75

Keeping black as a prod dependency

• Unpinning doesn't seem to be an option.
• In terms of versioning, I think the only option would be to pin it to the latest version

#78

pre-release

• If it has been fixed in later issues of pipenv, IMO, that would also mean this project would need to basically pin itself to >= the version of pipenv
  The last difference in pipenv versions I saw was between 2021.11.5 -> 2021.5.29 where the following regression occurred:
  pipenv 2021.11.5 regresses back to issue #2961 pypa/pipenv#4834
  haven't checked yet if this version has been yanked

Potential issues

• I think, while worth considering, a missing black installation would only expose a problem with the user's setup
  I'm not sure of the likelihood of people calling black without installing it themselves, as I think the only executable people would intend to find in this package is pipenv-setup

Proposed solution

my assumption is that as long as setup.py is OK - as the only (I think?) .py module this package directly alters - then black can be moved to dev packages
without having the knowledge you have at this time, I would say that from a distance the most ideal solutions would be for black to be a dev package

[bryant-finney]

@jshwi awesome, yeah it sounds like we're on the same page here 😊

• Re: package update, add missing dependency chardet #75 and relax the hard dependencies a bit #78: definitely agree
• Re: potential issues: yup, my thoughts as well!
• Re: proposed solution: I forgot to mention it, but I had assumed we would keep black pinned at 19.10b0 as a dev dependency

I think the only point that remains is whether to include extras like the following:

setup(
    extras_require={
        "autopep8": ["autopep8"],
        "black": ["black; python_version >= '3.6'"],
        "dev": [
            "autopep8~=1.4",
            "black==19.10b0; python_version >= '3.6'",
            "pytest~=5.2; python_version >= '3.6'",
            ...
        ],
    },
    ...
)

Any thoughts on this?

My motivation: it should allow us to provide a mitigation path for any projects that are somehow affected by this change. i.e. we could suggest

$ pip install "pipenv-setup[black]"
# or...
$ pipenv install "pipenv-setup[black]"

I know we would need to verify that pipenv-setup supports extras that aren't defined in Pipfile for this to work, but it should be straightforward to do so

[jshwi]

@bryant-finney

I think I need to inspect the code more before I can form a solid opinion. It appears as though black and autopep8 must be doing more than I first assumed. From what you're saying, I gather that they actually run under the execution of pipenv-setup when executed by the end user. These packages are not just formatting this repository alone.

Are you able to confirm what python files black and autopep8 work on (i.e. setup.py) when the user executes pipenv-setup?

Cheers!

[bryant-finney]

@jshwi yup, the relevant section is in the pipenv_setup.setup_updater module: the format_file() function uses either black or autopep8 to format the file. I have a working patch in my local clone, and I'm trying to add a new workflow for testing the installation and usage of the package

[jshwi]

@bryant-finney Ah, awesome, I'll check that out

[jshwi]

@bryant-finney All makes sense now.

Interesting, black is imported in the try, except block, but it isn't actually used, it is called via subprocess, whereas autopep8 is called directly. Edit: didn't realise it was try excepting the import. Maybe shutil.which could be called. Would black as a side effect be too chaotic?

I think the extras option for black is correct. Maybe keeping autopep8 as a permanent fallback is not such a bad idea. I don't think it needs to be an extra - might be more confusing with all those options?

A lot of people use black as the de-facto standard for python formatting, having the option there to keep everything consistent sounds good to me.

I can assume the big problem with removing black would be the case of CI where people didn't check setup.py before, because it was passing, and then suddenly seeing a failed build for a black --check, or something like that. but that seems unlikely, because that would mean they generally have black installed.

Eventually a try, except without any black extras could work, where it would incidentally run with any environment where black is installed.

[bryant-finney]

@jshwi

Maybe shutil.which could be called. Would black as a side effect be too chaotic?

I haven't verified yet, but I hope to actually use the black module retuned from this function. I'm hesitant to rely on the shell for executing in a subprocess: I've run into issues where a package entry point fails when it is executed from within a virtual environment. In other words:

# e.g. black is installed to the system python
(.venv) $ which black
/usr/local/bin/black

(.venv) $ head -n1 $(which black)
#!/usr/bin/env python

# in this scenario, calling black fails with an
# import error b.c. it is not included with the
# virtual environment

I don't think it needs to be an extra - might be more confusing with all those options?

Yeah I could go either way. It's been years since I've actually used autopep8 haha. I expect the original motivation for using it was to support Python <3.6; however, that's no longer necessary. How would you feel about just not specifying it as a dependency or extra? That makes more sense to me and would reduce dependencies

Eventually a try, except without any black extras could work, where it would incidentally run with any environment where black is installed.

Sweet that sounds exactly like what I'm thinking (see the linked function above). This would also allow us to add support for other formatters down the road (e.g. yapf) 😃

[jshwi]

I think its possible to call black.__main__.main, or it has a main function called _patched_main or something.
On that last point, sounds like there is a solution.
tbh, never got autopep8 to work a few years ago, so gave up on it. I like pep8, personally, so I wanted to make it work.
If black never releases this mythical stable release and pipenv can't find a compromise, a new formatter would be the go lol.

[bryant-finney]

@jshwi I think #88 is about setup now. It ended up taking a while to work out the usage.yml workflow, but I believe it tests what we want to test in order to close this issue.

When resolving the failing tests, I'll also take a swing at calling a function provided by black; it shouldn't take much 🙂

[jshwi]

@bryant-finney This will knock down a major barrier for using this package. Nice work!