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

fancy-queen-20734

01/23/2020, 6:41 PM
could I get a review on https://github.com/pantsbuild/pants/pull/9003 modulo CI et al
h

hundreds-father-404

01/23/2020, 6:47 PM
Nit: we’ve started using
assert foo == bar
in tests rather than
self.assertEqual
. Why? Pytest has really nice magic that shows how each of the values were derived. See https://docs.pytest.org/en/latest/assert.html It’s also nice to only ever have one
assert
rather than 40
self.assertX
, e.g.
assert "foo" in stdout
f

fancy-queen-20734

01/23/2020, 6:47 PM
oh cool.
I got use to that way back when I was writing Ruby, then also in Java with hamcrest
💯 1
it’s awesome that python has it too
h

hundreds-father-404

01/23/2020, 6:49 PM
Also, are you hoping to see the CI output on this one? If not, very much appreciated to cancel the CI run. If you expect that you’ll need more commits to get this landable, then appreciated to cancel any irrelelevant individual shards like “Rust tests” Our CI is atrociuously slow right now because migrating from travis-ci.org to travis-ci.com means that we only have 1 worker for the whole organization 😬 we’re still backed up from PRs last evening
f

fancy-queen-20734

01/23/2020, 6:49 PM
oof
h

hundreds-father-404

01/23/2020, 6:50 PM
Fingers crossed for a speedy response from Travis 🤞
f

fancy-queen-20734

01/23/2020, 6:53 PM
I was thinking I’d let the relevant bits run, but if you want changes I may as well cancel
Thanks for the quick look!
h

hundreds-father-404

01/23/2020, 6:56 PM
I don’t want any changes personally - didn’t have a chance to look closely more than a quick glance. Feel free to keep the relevant bits! Canceling irrelevant bits like Rust tests and Build wheels would be helpful, though
f

fancy-queen-20734

01/23/2020, 6:56 PM
would you prefer
Copy code
assert long-expr == \
other-expr
or
Copy code
assert (long-expr ==
other-expr)
h

hundreds-father-404

01/23/2020, 6:57 PM
Convention is the latter
f

fancy-queen-20734

01/23/2020, 6:57 PM
cool. That’s easier to s&r for
h

hundreds-father-404

01/23/2020, 6:57 PM
s&r? oh, swap and replace?
f

fancy-queen-20734

01/23/2020, 6:58 PM
search and replace
👍 1
wow the queue is long
h

hundreds-father-404

01/23/2020, 9:50 PM
And only getting more and more backlogged 😕
f

fancy-queen-20734

01/23/2020, 9:56 PM
at least I’ve canceled the irrelevant shards for my build
❤️ 1
w

witty-crayon-22786

01/24/2020, 5:42 PM
@fancy-queen-20734: sorry for the delay. looking today
f

fancy-queen-20734

01/24/2020, 6:10 PM
thanks
w

witty-crayon-22786

01/24/2020, 6:55 PM
@fancy-queen-20734: added a quick response to the description: haven't reviewed the body yet
back in a bit
f

fancy-queen-20734

01/24/2020, 7:30 PM
@witty-crayon-22786 replied.
looks like my travis run got caught in the exec changeover — restarting a couple shards
w

witty-crayon-22786

01/24/2020, 9:12 PM
replied. thanks!
should i look deeper now, or do you want to respond to this feedback before i do?
f

fancy-queen-20734

01/24/2020, 10:19 PM
respond to feedback I think. oh, for pushing it down, how would you feel about using a mixin vs doing more dynamic property checking?
@witty-crayon-22786 ^
w

witty-crayon-22786

01/24/2020, 10:28 PM
ahhhm. no huge preference on mixin vs property checking?
f

fancy-queen-20734

01/24/2020, 10:29 PM
I think I’d prefer a mixin because it’s more explicit, but I’ll see how complex that shakes out to be
Updated https://github.com/pantsbuild/pants/pull/9003 (introducing runtime_platform to jvm target types that pants execs) to use a mixin. I expect that CI should pass and I think it’s ready for review.
w

witty-crayon-22786

02/13/2020, 12:49 AM
thanks! will take a look tonight/tomorrow
Looks good! There is one confusing pair of methods, but other than that no blockers. Thanks.
4 Views