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 script to ease migration to black #3038

Merged
merged 2 commits into from Jun 1, 2022

Conversation

hbrunn
Copy link
Contributor

@hbrunn hbrunn commented Apr 27, 2022

Description

I was looking for a script to update existing feature branches automatically when introducing black in the main branch, but couldn't find any

To test, you need to blackify your main branch, commit that, switch to a feature branch and then run the script. You'll end up with a copy of your branch blackified and rebased on the main branch

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

I actually did none of the above but happily will update the docs if you think it's a good idea to have such a script here in the first place.

@cooperlees
Copy link
Collaborator

cooperlees commented Apr 27, 2022

Thanks for the PR. The idea is great, but I feel if we were to add this we might need to think about it supporting some more configuration and support all Operating Systems - e.g. Windows. So maybe this could be written in Python using subprocess + we add some testing.

What do others think?

@hbrunn
Copy link
Contributor Author

hbrunn commented Apr 27, 2022

I'm not others, but I think you're right the script should work on whatever platform black supports, and didn't think of windows (while taking care this is going to work on any unixoids, ie this should work on Macos, BSD, etc even though I use some Linux).

Converting this to pure Python is simple enough, but will take time as that's then my private fun and not customer's time.

I'm not very familiar with current windows stuff, can we expect a patch command there? Or should this pull another python library then to do it?

What kind of configuration do you have in mind?

@ichard26 ichard26 self-requested a review Apr 27, 2022
@hbrunn
Copy link
Contributor Author

hbrunn commented Apr 30, 2022

I just pushed a pure python version of this, also replacing patch with git apply

1 similar comment
@hbrunn
Copy link
Contributor Author

hbrunn commented Apr 30, 2022

I just pushed a pure python version of this, also replacing patch with git apply

@jack1142
Copy link
Contributor

jack1142 commented Apr 30, 2022

I could probably check but do you know if this script retains the original author (and maybe committer?) date? And I guess the author/committer themselves too.

@hbrunn
Copy link
Contributor Author

hbrunn commented Apr 30, 2022

that's what git commit -C $commit does

Copy link
Collaborator

@cooperlees cooperlees left a comment

Thanks for this. This is much friendlier to all operating systems.

Only things I think we could do here:

  • Should we check that we have a new enough git or are all these arguments been around a long time?
  • Lets log black output to a file with the logfile from an option argparse argument
  • Maybe also specify you need git + black in your PATH for this to work
    • Optionally we could check for that
  • Lets type annotate - Again - If you don't have time I can do once we merge ...

scripts/migrate-black.py Show resolved Hide resolved
scripts/migrate-black.py Show resolved Hide resolved
current_branch = git("branch", "--show-current")

if not current_branch or base_branch == current_branch:
print("You need to check out a feature brach to work on")
Copy link
Collaborator

@cooperlees cooperlees Apr 30, 2022

Choose a reason for hiding this comment

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

Generally logging message I recommend using a logger from the logging module and writing to stderr - But we can clean that up in a follow up diff.

scripts/migrate-black.py Show resolved Hide resolved
Copy link

@StefanRijnhart StefanRijnhart left a comment

As a member of @hbrunn's team, I just tested this script on some of our open PR branches and it works really well.

@hbrunn please check the linting of the script itself, and it looks like you'll want to add that changelog entry indeed.

@hbrunn
Copy link
Contributor Author

hbrunn commented May 7, 2022

thanks for the reviews, now correctly formatted, using logging and with a changelog entry

@jack1142
Copy link
Contributor

jack1142 commented May 23, 2022

I think you should probably use an underscore (_) instead of a hyphen (-) in the name of the script (migrate-black.py) since migrate-black is not a valid module name (not much of an issue for a script but still) and it's consistent with what's already in that folder.

Not sure if it's a particularly great name for the script since I personally would find it useful for reformatting PRs I made that are incorrectly formatted, not just for migrating existing feature branches during migration.

@hbrunn
Copy link
Contributor Author

hbrunn commented May 24, 2022

Not sure if it's a particularly great name for the script since I personally would find it useful for reformatting PRs I made that are incorrectly formatted, not just for migrating existing feature branches during migration.

I'll be happy to rename this, what do you propose?

Copy link
Collaborator

@cooperlees cooperlees left a comment

I feel the name is fine. Lets just type annotate to get CI passing and we can merge this to have a starting place and let others improve it as people find time to do so.

e.g. unittests ...

CHANGES.md Outdated Show resolved Hide resolved
from subprocess import check_output, run, Popen, PIPE


def git(*args):
Copy link
Collaborator

@cooperlees cooperlees May 26, 2022

Choose a reason for hiding this comment

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

mypy CI is failing - Can you please type annotate the code since we have very high type coverage elsewhere.

Copy link
Contributor Author

@hbrunn hbrunn May 27, 2022

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

@cooperlees cooperlees left a comment

Close, but one last error ... Lets prefer to use types from the typing library :)

It's easy to run mypy locally or run pre-commit to run the CI

scripts/migrate-black.py Outdated Show resolved Hide resolved
scripts/migrate-black.py Outdated Show resolved Hide resolved
hbrunn and others added 2 commits Jun 1, 2022
Co-authored-by: Cooper Lees <me@cooperlees.com>
@hbrunn
Copy link
Contributor Author

hbrunn commented Jun 1, 2022

thanks, I actually forgot to activate pre-commit here, which is quite damning in exactly this project.

We've both been wrong about the type hinting for *args though

Copy link
Collaborator

@cooperlees cooperlees left a comment

Thanks for the persistent effort here. I hope people find it useful.

I guess the only way people might find it is docs - so maybe we should add a parapgraph here (with usage examples): https://github.com/psf/black/blob/main/docs/guides/introducing_black_to_your_project.md

Happy for this to be in another PR.

from subprocess import check_output, run, Popen, PIPE


def git(*args: str) -> str:
Copy link
Collaborator

@cooperlees cooperlees Jun 1, 2022

Choose a reason for hiding this comment

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

Wow, this must be new support from mypy :|

@cooperlees cooperlees merged commit 436e12f into psf:main Jun 1, 2022
37 checks passed
@hbrunn
Copy link
Contributor Author

hbrunn commented Jun 1, 2022

Happy for this to be in another PR.

please from a not-me-person. I might however provide just that because now I've to think about how to deliver that to my project, I expected some setup.py magic to put those scripts into $PATH but that's not the case

@cooperlees
Copy link
Collaborator

cooperlees commented Jun 1, 2022

This script is not included in the black distribution nor do I think it should be. I think we should just document it as an best effort supported helper script in our documentation that lives in the repo here.

As it has no externals deps, install is just - wget https://github.com/psf/black/blob/main/scripts/migrate-black.py && chmod +x migrate-black.py providing a python3 executable is in path ...

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

4 participants