https://pantsbuild.org/ logo
#development
Title
# development
w

witty-crayon-22786

06/19/2019, 6:28 PM
i think that that is an OK starter. but i think that it is complexified by not being entirely on one side of the rust/python boundary
which it sounds like you've learned a little bit more about
i'll see whether there is something with fewer moving parts to look at first
👌 1
h

hundreds-breakfast-49010

06/19/2019, 6:29 PM
eh, I can take a look at that, I need ot understand the python-rust boundary at some point
w

witty-crayon-22786

06/19/2019, 6:29 PM
so why not day three!?
=D
this one is probably more rusty: https://github.com/pantsbuild/pants/issues/7745
and deals less with the boundary (although possibly a little bit)
h

hundreds-father-404

06/19/2019, 6:33 PM
Good call. I just un-assigned myself. I wasn’t getting to that one for a while
w

witty-crayon-22786

06/19/2019, 6:38 PM
cleaned up the description a bit. it's possible that doing what Daniel originally suggested there is the right call, because we might lose diagnostic information otherwise.
👍 1
h

hundreds-breakfast-49010

06/20/2019, 9:25 PM
okay, so I took a stab at creating a wrapper type that didn't have the
description
field: https://github.com/gshuflin/pants/commit/82e52901c3a5b52f99dd4e7389a535b508243591
this doesn't compile right now, because I'm not sure how/where to add the description back, to turn my wrapper type back into an
ExecuteProcessRequest
in general, if you memoize this type, you have to lose the description information and get it back from some other source, right? unless I'm misunderstanding how this all works
w

witty-crayon-22786

06/20/2019, 9:28 PM
Yea
The "other source" that I was handwaving about was essentially just the other Node that requests the process execution Node
But.... Yea, I think that until we know what UX changes we want to make (see the discussion in the other thread with Borja) it's not clear how we will render the description anyway
It night be good to just stick with changing `Eq`/`Hash` for now
h

hundreds-breakfast-49010

06/20/2019, 9:33 PM
I glanced at that thread, but wasn't sure what to make ofit
w

witty-crayon-22786

06/20/2019, 9:34 PM
Well, definitely run the command I included there and get a feel for what the v2 UI looks like
h

hundreds-breakfast-49010

06/20/2019, 9:35 PM
okay, so, for now, just change Eq/Hash on
ExecuteProcessRequest
such that they ignore the description field?
👍 1
h

hundreds-father-404

06/20/2019, 9:38 PM
okay, so, for now, just change Eq/Hash on
ExecuteProcessRequest
such that they ignore the description field?
I think yes. I would maybe include a
TODO(#issue-number):
to revisit this design once the UX approach is figured out.
h

hundreds-breakfast-49010

06/20/2019, 9:49 PM
kk
w

witty-crayon-22786

06/20/2019, 10:04 PM
@hundreds-breakfast-49010: did Benjy share this with you: https://drive.google.com/file/d/1g1dqLXsTl7uMI-M7_1WFlkbbEoZbbNzx/view?usp=drivesdk ?
h

hundreds-breakfast-49010

06/20/2019, 10:04 PM
nope, will watch
w

witty-crayon-22786

06/20/2019, 10:04 PM
👍
h

hundreds-breakfast-49010

06/21/2019, 12:04 AM
I have https://github.com/pantsbuild/pants/pull/7916 but I'd like to write a test for this, I'm not sure what the most concise way to set up a testable situation that will exercise this code is
h

hundreds-father-404

06/21/2019, 12:09 AM
Canceled CI since you’ll be adding a test (draft PRs are helpful in situations like this) The changes look good! Nice it’s such a small diff. For testing, what about a unit test that creates two
ExecuteProcessRequest
where everything is the same, except for the description, and confirm that they are equal.
h

hundreds-breakfast-49010

06/21/2019, 12:28 AM
are draft PRs a github thing?
ah yeah a unit test that just tests the one feature would work, I'll add that to the crate
👌 1
h

hundreds-father-404

06/21/2019, 12:29 AM
Yes, when you’re opening up the PR, click the dropdown button on the big green button to create a draft. They won’t trigger CI until you mark it as ready, and I think send less notifications? https://github.blog/2019-02-14-introducing-draft-pull-requests/
h

hundreds-breakfast-49010

06/21/2019, 12:29 AM
I was thinking more, how do I write a more full-featured pants test that would exercise this?
ah that's relatively new
useful feature
h

hundreds-father-404

06/21/2019, 12:31 AM
I was thinking more, how do I write a more full-featured pants test that would exercise this?
Not sure if others would agree, but here I don’t think the cost of an integration test—in terms of the time it takes for you to write, the extra code we have to maintain, and the time to run it every time in CI—would be worth it. We in general really like integration tests and use them a lot, but I think a unit test suffices here.
w

witty-crayon-22786

06/21/2019, 12:39 AM
If you're able to test by generating a very large random number using bash twice with different descriptions... You could do this in
test_isolated_process.py
Integration tests are pretty valuable, and a fast, stable integration test is definitely more useful than a fast stable unit test
(it's just making them fast and stable that is challenging!)
💯 1
h

hundreds-breakfast-49010

06/21/2019, 12:43 AM
like many things in engineering, it's a tradeoff between useful goals 🙂