https://pantsbuild.org/ logo
#general
Title
# general
f

fancy-motherboard-24956

10/07/2019, 5:37 PM
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

happy-kitchen-89482

10/07/2019, 5:38 PM
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

fancy-motherboard-24956

10/07/2019, 5:40 PM
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

happy-kitchen-89482

10/07/2019, 5:42 PM
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

early-needle-54791

10/07/2019, 5:44 PM
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

fancy-motherboard-24956

10/07/2019, 5:44 PM
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

aloof-angle-91616

10/07/2019, 5:45 PM
we've got quite a lot of forks... https://github.com/pantsbuild/
e

early-needle-54791

10/07/2019, 5:45 PM
not necessarily a justification for more.
👍 2
are we all making decisions with the entire community in mind, or just our personal preferences?
f

fancy-motherboard-24956

10/07/2019, 5:46 PM
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

early-needle-54791

10/07/2019, 5:47 PM
I guess just speaking based on my experience using 3rd party software, not claiming to know what is best for the pants community.
a

aloof-angle-91616

10/07/2019, 5:47 PM
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

hundreds-father-404

10/07/2019, 5:47 PM
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

fancy-motherboard-24956

10/07/2019, 5:47 PM
Especially buried in this thread is probably not very visible
a

aloof-angle-91616

10/07/2019, 5:47 PM
a single-line change is not significant overhead unless i'm misunderstanding something?
e

early-needle-54791

10/07/2019, 5:50 PM
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

happy-kitchen-89482

10/07/2019, 5:51 PM
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

witty-crayon-22786

10/07/2019, 5:51 PM
re: 2 vs 4, i would not like to use a fork of black.
1
f

fancy-motherboard-24956

10/07/2019, 5:51 PM
Yes, we should find a good fix for the triple string issue
h

happy-kitchen-89482

10/07/2019, 5:52 PM
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

witty-crayon-22786

10/07/2019, 5:52 PM
so should we revert until the instructions are fixed?
h

happy-kitchen-89482

10/07/2019, 5:52 PM
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

fancy-motherboard-24956

10/07/2019, 5:52 PM
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

witty-crayon-22786

10/07/2019, 5:52 PM
yep, sounds good.
h

hundreds-father-404

10/07/2019, 5:53 PM
Yes I think revert and make the instructions more robust. Also we should have
black.py
merged before the big change
h

happy-kitchen-89482

10/07/2019, 5:53 PM
Sounds good, thanks.
w

witty-crayon-22786

10/07/2019, 5:53 PM
@fancy-motherboard-24956: do you want to do that, or should we be able to do it?
so that you can go home
a

aloof-angle-91616

10/07/2019, 5:53 PM
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

fancy-motherboard-24956

10/07/2019, 5:53 PM
black.py is already merged, Eric
h

hundreds-father-404

10/07/2019, 5:53 PM
*build-support/bin/black.py
👍 1
f

fancy-motherboard-24956

10/07/2019, 5:53 PM
ah ok
yeah, all but the hook can go on its own, I guess
h

happy-kitchen-89482

10/07/2019, 5:54 PM
I mean, I think the triple-string issue is a one-off issue?
1
w

witty-crayon-22786

10/07/2019, 5:54 PM
@fancy-motherboard-24956: shall i do the revert, or are you starting it?
f

fancy-motherboard-24956

10/07/2019, 5:54 PM
w

witty-crayon-22786

10/07/2019, 5:54 PM
ok, thank you!
f

fancy-motherboard-24956

10/07/2019, 5:55 PM
Yes, it’s a one off issue for now and support may end up in black in the future
w

witty-crayon-22786

10/07/2019, 5:55 PM
and thanks for the flexibility
❤️ 1
h

happy-kitchen-89482

10/07/2019, 5:55 PM
Maybe the docformatter can also deal with triple-strings that aren't docstrings?
f

fancy-motherboard-24956

10/07/2019, 5:55 PM
Yeah, I’ll test this hypothesis
h

happy-kitchen-89482

10/07/2019, 5:55 PM
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

fancy-motherboard-24956

10/07/2019, 5:56 PM
and thanks for the flexibility
Pleasure. Sorry for breaking you all and misunderstanding which communication channels were appropriate
w

witty-crayon-22786

10/07/2019, 5:56 PM
on the bright side, v2 caches the rewrite, so the second time round it takes ~8 seconds 😃
❤️ 1
h

happy-kitchen-89482

10/07/2019, 5:56 PM
I too appreciate your flexibility.
❤️ 2
How long did the uncached one take?
w

witty-crayon-22786

10/07/2019, 5:58 PM
a few minutes. it invokes a (python) process per target.
f

fancy-motherboard-24956

10/07/2019, 5:59 PM
Yes, like 2-3 minutes
and black alone would take about 30 seconds
h

hundreds-father-404

10/07/2019, 5:59 PM
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

fancy-motherboard-24956

10/07/2019, 5:59 PM
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

witty-crayon-22786

10/07/2019, 6:00 PM
there are distinct advantages to not batching. for one, you cache hit per process
👍 2
f

fancy-motherboard-24956

10/07/2019, 6:03 PM
That’s the revert PR. May I let you monitor it, Stu? I’ll be heading home for the day
w

witty-crayon-22786

10/07/2019, 6:04 PM
yep!
have a good night.
i'm going to go ahead and merge without waiting for CI
👌 2
f

fancy-motherboard-24956

10/07/2019, 6:06 PM
Without waiting for CI? That took some courage 🧌
Thanks everyone. Today I Learned… a bunch of things 😄
😁 1
❤️ 2
2 Views