https://pantsbuild.org/ logo
Join Slack
Powered by
t
message has been deleted
w
Example lightweight, short-form: ā€¢ "What backends do you predominantly use?" [Multi select] ā€¢ "Which theme of work [blah blah]" ā€¢ "Any free-form comments"?
Example context for call-by-name:
Copy code
my_foo = await Get(Foo, FooRequest, value) 

# becomes something like

my_foo = await some_function(value) 

# or 

my_foo = await some_function(value, **implicitly())

# or ...
šŸ‘ 1
w
I have also bitten the recursive bullet in my typescript backend adventures btw
w
šŸ˜¢
w
@wide-midnight-78598 Is there a recipe for 
MockGet
migration?
w
Nope, not yet - I'm not looking at the test code until the primary code migration is done. Also, I think mockget is used about 30 times in the repo or something? So, blissfully ignorant of it at the moment
w
I did a test on the js backend and of course I've used it šŸ˜… I mean you cant really migrate the code without migrating the tests?
w
You can't? We still have the normal rule runner in place
w
You mean leave the callsites that are mocked, then?
The auto migrator produces failing tests, is what I mean
w
Whoa, really? Where?
w
Any place MockGet is used, I assume? Specifically 
nodejs_test.py
w
Interesting - I ran the migration over JS and TS, but didn't hit any failing tests
From what I can tell, Node, Docker, AWS, and a couple Python backends use MockGet - and none have been migratied and merged yet - so, I can't speak too reliably about them
w
Maybe it skipped some file? I've manually corrected:
Copy code
19:08:13.46 [WARN] Failed to migrate Get (2, <class 'libcst._nodes.expression.Call'>) in src/python/pants/backend/javascript/subsystems/nodejs.py:node_process_environment due to: Expected a Name but got a Attribute!
and then re-applied the migration
w
Failing to migrate is just a warning, that shouldn't fail anything - it'll just keep the Get syntax
w
It skips the file though
or maybe all 
Get
lines failed in the file, I didn't read too closely
w
I can look at this tonight when I'm back home. Was gonna pull the latest JS code and re-migrate today or tomorrow anyways
You can check out the migration plan (generated from the rust code rule generator stuff) using 
--json
I think? 
pants help migrate-call-by-name
has the syntax, so you can see what we operate on
Ohhhh
I know why that warning pops up... Ugh, it was something I noticed while running some tests - when we have chained 
x.y
- those are not 
cst.Name
but rather 
cst.Attribute
and you have to do some recursion to get the fully qualified value. Shouldn't affect the tests though - I have to update the migration tool to cover this use case as well, since we use it a few times in the codebase
w
I see. But to circle back to my observation and question: MockGet doesn't work with call by name, and MockGet can be anywhere in tests and they will fail if the migrator / a user changes to call by name. There is no known fix other than to revert.
Addendum is MockGet is used sparingly and their handling is not blocking to the larger migration, of course.
w
At this moment, correct I guess. I have spent no substantial time on MockGet yet - as I've ignored test files. If a call-by-name migration to a production source file causes a MockGet'ted test to fail - then yes, the production code should be reverted, and this will be something I need to look at sooner
šŸ‘ 1
w
I've dug some and I think if 
PyGeneratorResponseCall
exposes the same variables as 
PyGeneratorResponseGet
(args and output type) could be a solution!
Yes that works. Not sure how we feel about this long term but seems cheap enough short term!
10 Views
Open in Slack
PreviousNext
Powered by