https://pantsbuild.org/ logo
Join Slack
Powered by
<@UKPS08ZGW>: re <https://github.com/pantsbuild/pa...
# development
w
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
eh, I can take a look at that, I need ot understand the python-rust boundary at some point
w
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
Good call. I just un-assigned myself. I wasn’t getting to that one for a while
w
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
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
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
I glanced at that thread, but wasn't sure what to make ofit
w
Well, definitely run the command I included there and get a feel for what the v2 UI looks like
h
okay, so, for now, just change Eq/Hash on 
ExecuteProcessRequest
such that they ignore the description field?
👍 1
h
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
kk
w
@hundreds-breakfast-49010: did Benjy share this with you: https://drive.google.com/file/d/1g1dqLXsTl7uMI-M7_1WFlkbbEoZbbNzx/view?usp=drivesdk ?
h
nope, will watch
w
👍
h
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
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
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
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
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
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
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
like many things in engineering, it's a tradeoff between useful goals 🙂
Open in Slack
PreviousNext
Powered by