https://pantsbuild.org/ logo
Join Slack
Powered by
could I get a review on <https://github.com/pantsb...
# general
f
could I get a review on https://github.com/pantsbuild/pants/pull/9003 modulo CI et al
h
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
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
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
oof
h
Fingers crossed for a speedy response from Travis 🤞
f
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
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
would you prefer
Copy code
assert long-expr == \
other-expr
or
Copy code
assert (long-expr ==
other-expr)
h
Convention is the latter
f
cool. That’s easier to s&r for
h
s&r? oh, swap and replace?
f
search and replace
👍 1
wow the queue is long
h
And only getting more and more backlogged 😕
f
at least I’ve canceled the irrelevant shards for my build
❤️ 1
w
@fancy-queen-20734: sorry for the delay. looking today
f
thanks
w
@fancy-queen-20734: added a quick response to the description: haven't reviewed the body yet
back in a bit
f
@witty-crayon-22786 replied.
looks like my travis run got caught in the exec changeover — restarting a couple shards
w
replied. thanks!
should i look deeper now, or do you want to respond to this feedback before i do?
f
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
ahhhm. no huge preference on mixin vs property checking?
f
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
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
Open in Slack
PreviousNext
Powered by