Skip to content

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Aug 12, 2022

Limited cherry-pick of #109428 that only has the deprecation method replacement without the other lint fixes. Limited the cp to just this line to reduce risk.

Re lack of testing:

We have good test coverage of both these methods, but they are running on a stable version of Ruby in our CI (I think it's just using the macOS default system version 2.6).
Getting a preview version of Ruby into our CI system doesn't seem feasible.

Fixes #109476

@jmagman jmagman added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 12, 2022
@jmagman jmagman self-assigned this Aug 12, 2022
@flutter-dashboard flutter-dashboard bot changed the base branch from flutter-3.2-candidate.5 to master August 12, 2022 20:43
@flutter-dashboard
Copy link

This pull request was opened against a branch other than master. Since Flutter pull requests should not normally be opened against branches other than master, I have changed the base to master. If this was intended, you may modify the base back to flutter-3.2-candidate.5. See the Release Process for information about how other branches get updated.

Reviewers: Use caution before merging pull requests to branches other than master, unless this is an intentional hotfix/cherrypick.

@flutter-dashboard flutter-dashboard bot added engine flutter/engine related. See also e: labels. framework flutter/packages/flutter repository. See also f: labels. labels Aug 12, 2022
@jmagman jmagman changed the base branch from master to flutter-3.2-candidate.5 August 12, 2022 20:45
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@jmagman jmagman removed framework flutter/packages/flutter repository. See also f: labels. engine flutter/engine related. See also e: labels. labels Aug 12, 2022
@jmagman
Copy link
Member Author

jmagman commented Aug 12, 2022

If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat

See test-exemption from original PR #109428 (comment)

test-exempt: for practical reasons. testing this is likely to cost more than it saves since it's an entirely new ecosystem. #109431 in case we change out minds.

@itsjustkevin itsjustkevin self-requested a review August 17, 2022 18:50
@muditatandon
Copy link

Hey @jmagman can this be merged or luci-flutter test retried as that is the one which is failing currently.

@jmagman
Copy link
Member Author

jmagman commented Aug 22, 2022

Hey @jmagman can this be merged or luci-flutter test retried as that is the one which is failing currently.

I'm not sure where the luci-flutter failing state is coming from, it should reflect the state of the tree. I filed #110023

@jmagman
Copy link
Member Author

jmagman commented Aug 23, 2022

Closing, #110045 was merged.

@jmagman jmagman closed this Aug 23, 2022
@jmagman jmagman deleted the cp-ruby-exists branch August 23, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants