https://pantsbuild.org/ logo
Join Slack
Powered by
Let’s start a new thread here to discuss difficult...
# general
f
Let’s start a new thread here to discuss difficulties after merging the black PR (sorry; I’m a bit lost between the various other threads)
👍 2
So there are at least 2 issues: • The steps I gave to rebase a branch don’t work as expected (still investigating this) • docstrings
h
My summary of the current situation: It looks like the fact that black can't reformat triple-quoted strings is a real problem. At least for the transition. I have been unable to resolve merge conflicts in a principled way, using Pierre's instructions, because of it. At least part of the problem is that during the merge the pants sources are broken, so running black via pants as part of the interactive rebase doesn't work.
👍 1
f
I think it’s a problem for consistency (and I’m not 100% sure yet whether docformatter addresses all of it or only part of it [it seems it may be the latter]) but my steps are broken for another reason, because with manual black and git checkout, I can get the effect I want; so the fact my steps don’t work comes from an error in them.
(See https://github.com/pierrechevalier83/pants/tree/benjyw/provenanced_addresses2 for an example of what the steps I gave should output, but don’t <- involved some manual listing of files, so could be incorrect)
h
Our options include: 1. Push forward. Change the merge conflict instructions to run black directly, instead of via pants, during merge conflict resolution. And reformat all our triple-strings, somehow... 2. Revert, and try again with Danny's 2-space fork of Black (known henceforth as 
black2
). This eliminates the triple-string problem.
👍 1
Thoughts?
e
I think it would be better to stay on the upstream black if we do use it. My understanding of danny's PR is that it is a fork and source code change.
👍 2
(correct me if im wrong)
f
Two thoughts from here: * maintaining a fork of black for 2 spaces is overhead that doesn’t seem worth the hassle in the long run. I think the end point of simply using normal black is a better thing to aim for. * if my PR is being painful and I can’t come up with better steps for people in a short time, we should revert it independently of the 2 vs 4 spaces discussion; and I can give it a shot later after addressing the things that are blocking.
a
we've got quite a lot of forks... https://github.com/pantsbuild/
e
not necessarily a justification for more.
👍 2
a
are we all making decisions with the entire community in mind, or just our personal preferences?
f
Yes, the 2 vs 4 spaces discussion should probably happen on pants-devel to avoid repeating the error that caused some of the hassle here
e
I guess just speaking based on my experience using 3rd party software, not claiming to know what is best for the pants community.
a
it's not a "justification" for more, but if we already have an established practice of maintaining forks of relevant code, and the fork here is a single-line change, i'm not sure the argument that maintaining a fork is difficult is a very appropriate one here.
h
My concern with a fork is having to keep it up-to-date. It’s more overhead for us Question: Danny, with your PR, would end Pants users be able to choose between upstream vs our fork?
f
Especially buried in this thread is probably not very visible
a
a single-line change is not significant overhead unless i'm misunderstanding something?
e
The overhead would be to keep track of rebasing and pushing a new "release" to pypi or wherever when we want to get some new feature from upstream. You're right its not a difficult task, just more mental load, and stuff to remember
➕ 2
h
Well, I for one prefer 2 spaces to 4 spaces, but I would still prefer even more to use upstream black if we can. So I don't want my 2 space preference to be a factor here. What is a factor, though, is this triple-string issue.
➕ 4
w
re: 2 vs 4, i would not like to use a fork of black.
➕ 1
f
Yes, we should find a good fix for the triple string issue
h
In other words, if we had a solution for the triple-string issue, I would advocate for 4 spaces, against my personal preference, just so we're not on a fork of black, even if it's a trivial one.
➕ 2
w
so should we revert until the instructions are fixed?
h
But if we don't have that solution, then "black2" seems viable to me.
It's not just the instructions, it's that we don't have a solution for reformatting triple-strings.
f
Why don’t we revert, and I’ll spend some time trying to come up with better steps and think of the docstrings? We can also have some discussions on pants-devel for 2 vs 4 spaces and for the change in general, which will give us an opportunity to properly notify the community?
w
yep, sounds good.
h
Yes I think revert and make the instructions more robust. Also we should have 
black.py
merged before the big change
h
Sounds good, thanks.
w
@fancy-motherboard-24956: do you want to do that, or should we be able to do it?
so that you can go home
a
i'll get over it because i care more about pants than about spacing. i'm glad we're discussing this transition now with appropriate stakeholders instead of retroactively justifying it.
f
black.py is already merged, Eric
h
*build-support/bin/black.py
👍 1
f
ah ok
yeah, all but the hook can go on its own, I guess
h
I mean, I think the triple-string issue is a one-off issue?
➕ 1
w
@fancy-motherboard-24956: shall i do the revert, or are you starting it?
f
w
ok, thank you!
f
Yes, it’s a one off issue for now and support may end up in black in the future
w
and thanks for the flexibility
❤️ 1
h
Maybe the docformatter can also deal with triple-strings that aren't docstrings?
f
Yeah, I’ll test this hypothesis
h
We have quite a lot of those
Thanks for working through this. A change this massive was inevitably going to hit some snags...
❤️ 1
f
and thanks for the flexibility
Pleasure. Sorry for breaking you all and misunderstanding which communication channels were appropriate
w
on the bright side, v2 caches the rewrite, so the second time round it takes ~8 seconds 😃
❤️ 1
h
I too appreciate your flexibility.
❤️ 2
How long did the uncached one take?
w
a few minutes. it invokes a (python) process per target.
f
Yes, like 2-3 minutes
and black alone would take about 30 seconds
h
One of the possible changes would be to batch run Black, rather than one invoke per target. It’s designed to only be invoked once
f
I thought that was acceptable for now as we would mostly not run on full sources after the first time. Was also considering batching later
(especially if we want to format the twitter source code some day)
w
there are distinct advantages to not batching. for one, you cache hit per process
👍 2
f
That’s the revert PR. May I let you monitor it, Stu? I’ll be heading home for the day
w
yep!
have a good night.
i'm going to go ahead and merge without waiting for CI
👌 2
f
Without waiting for CI? That took some courage 🧌
Thanks everyone. Today I Learned… a bunch of things 😄
😁 1
❤️ 2
2 Views
Open in Slack
PreviousNext
Powered by