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

Add --projects cli flag to black-primer #2555

Merged
merged 4 commits into from Oct 27, 2021
Merged

Conversation

nipunn1313
Copy link
Contributor

@nipunn1313 nipunn1313 commented Oct 22, 2021

Description

Makes it possible to run a subset of projects on black primer

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary? (Added tests!)
  • Add new / update outdated documentation? (CLI documentation gets inlined)

@nipunn1313 nipunn1313 force-pushed the black_primer branch 2 times, most recently from 4edcd96 to 553faf0 Compare October 22, 2021 01:08
src/black_primer/lib.py Outdated Show resolved Hide resolved
src/black_primer/cli.py Outdated Show resolved Hide resolved
src/black_primer/lib.py Outdated Show resolved Hide resolved
Makes it possible to run a subset of projects on black primer
src/black_primer/cli.py Show resolved Hide resolved
@hugovk
Copy link
Contributor

hugovk commented Oct 22, 2021

Perhaps name this --include, to leave the option open to add a symmetrical --exclude in the future?

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Thanks for the idea /addition. I feel this is already achievable tho. Via config files.

Whats the use case and advantage of this over just making a dedicated subset config file(s)?

  • I guess so you can have a large config file and only run some projects

But I feel if you're doing that a lot just having multiple config files would also work. But I get de-duplication is nice.

Interested to hear so I can understand more. Thanks!

@@ -26,6 +27,8 @@
_timestamp = datetime.now().strftime("%Y%m%d%H%M%S")
DEFAULT_WORKDIR = Path(gettempdir()) / f"primer.{_timestamp}"
LOG = logging.getLogger(__name__)
DEFAULT_CONFIG_CONTENTS = json.load(open(DEFAULT_CONFIG))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get why you're doing this, but this is bad import time file opening ... and no close of the file descriptor ... But no one should ever import cli.py ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was added so that black-primer --help showed the projects. I wonder if click supports delayed execution of the default value. If so, might be possible to do better. Let me poke at it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do something like this:

@click.option(
    "--projects",
    default=lambda: load_projects(DEFAULT_CONFIG),
    callback=_projects_callback,
    show_default=f"See {DEFAULT_CONFIG}",
    help="Comma separated list of projects to run",
)

which shows

  --projects TEXT        Comma separated list of projects to run  [default:
                         (See /Users/nipunn/src/black/src/black_primer/primer.
                         json)]

Alternately can do

@click.option(
    "--projects",
    default=load_projects(DEFAULT_CONFIG),
    callback=_projects_callback,
    show_default=True,
    help="Comma separated list of projects to run",
)

which shows

  --projects TEXT        Comma separated list of projects to run  [default:
                         STDIN, aioexabgp, attrs, bandersnatch, channels,
                         cpython, django, flake8-bugbear, hypothesis, pandas,
                         pillow, poetry, ptr, pyanalyze, pyramid, pytest,
                         scikit-lego, sqlalchemy, tox, typeshed, virtualenv,
                         warehouse]

But that ends up still executing the open at import time - since click.option's args are executed at import time. I think based on the way click works, the help page's contents needs to exist at import time, so the config must be opened early. I think opening the config at import time is a minor price to pay for this convenience - I could leave a comment to that extent.

@nipunn1313
Copy link
Contributor Author

I'll tell you my use case -

I was writing an improvement for black, and I wanted to check out how it would do using black-primer. Notably, black-primer in CI ran on 22 projects and came up with some issues. I wanted to repro by running black-primer locally on one of those projects, and was surprised to find that black-primer ran all-or-nothing.

It clearly can be done by making a new config file / editing the config file, but that seemed cumbersome (it took me a while to figure out where black-primer's config was coming from). Particularly to a new developer to the project (like me), I thought it was tough to address the CI feedback from black-primer. Using --projects (or --include as someone else suggested) seemed like an improvement. In my opinion it seems like a good way to go.

@hugovk
Copy link
Contributor

hugovk commented Oct 25, 2021

I'll tell you my use case -

I was writing an improvement for black, and I wanted to check out how it would do using black-primer. Notably, black-primer in CI ran on 22 projects and came up with some issues. I wanted to repro by running black-primer locally on one of those projects, and was surprised to find that black-primer ran all-or-nothing.

It clearly can be done by making a new config file / editing the config file, but that seemed cumbersome (it took me a while to figure out where black-primer's config was coming from). Particularly to a new developer to the project (like me), I thought it was tough to address the CI feedback from black-primer. Using --projects (or --include as someone else suggested) seemed like an improvement. In my opinion it seems like a good way to go.

Aha, so the option replaces the list from config. I misread it as running projects in addition to the list from config. So perhaps --projects is in fact better to begin with! Sorry for the noise! (Another idea, --select like Flake8.)

@nipunn1313
Copy link
Contributor Author

I don't think it's worth bikeshedding on the name. black-primer is only used by black devs and it would be pretty easy to change it in the future. It's a very reversible decision - since it's not too critical to keep compatibility. Don't think we should overthink it.

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Sure, agree. This is indeed a dev tool.

@cooperlees cooperlees merged commit 467efe1 into psf:main Oct 27, 2021
@ichard26
Copy link
Collaborator

ichard26 commented Oct 28, 2021

Does the new test_projects test really need to run black-primer fully? It's so slow that it's doubling my local pytest run (adding 20-25 seconds to a run which normally takes 20-30 seconds) which is not great ...

@nipunn1313
Copy link
Contributor Author

It's only running a single project, but probably it's not needed if I can mock something out appropriately. It only took 8 seconds for me, but even that is far too long. Lets me see what I can do. Thanks for pointing it out.

JelleZijlstra pushed a commit that referenced this pull request Nov 16, 2021
* Add --projects cli flag to black-primer

Makes it possible to run a subset of projects on black primer

* Refactor into click callback
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.

None yet

5 participants