-
Notifications
You must be signed in to change notification settings - Fork 7
update tool/generate_* scripts and sync files generated by those scripts #150
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looking good so far.
Looks good to me--is it ready for review/merge? Thanks! |
I didn't fix the TODO you've mentioned on discord https://github.com/dart-lang/macros/blob/66baf8b8266d393d2034f02d898723e9318f3ab4/pkgs/_test_macros/lib/built_value.dart#L186 But if I can do it separately then yes the PR is ready! |
Hmmm it should be a trivial change now I think--just check the name? Then, it's a good e2e test of the PR :) Or if it's more work, sure, another PR sounds good. |
should be ok now, I've also updated the test case |
e0d92e2
to
adb2a64
Compare
No worries, that's what we have presubmit checks for :) let's see if they turn green this time... |
Fixed the typo and updated goldens, I did run mono_repo analyze && test and I do have a single test failing in
|
Looks great, thanks :) |
there are some changes I'm not sure should be there? see my comments
cc @davidmorgan