Skip to content

Conversation

@mraleph
Copy link
Member

@mraleph mraleph commented Feb 8, 2018

This allows const GlobalObjectKey(0) to be concisely formatted as
[GlobalObjectKey int#0000] in both Dart 2 and Dart 1 modes.

Without this change it would be formatted as
[GlobalObjectKey<State> int#0000] because in Dart 2
types are instantiated to bounds.

In addition to retaining general readability this also fixes few
tests that rely on this short string representation (see
test/widgets/global_keys_duplicated_test.dart).

This allows const GlobalObjectKey(0) to be concisely formatted as
[GlobalObjectKey int#0000] in both Dart 2 and Dart 1 modes.

Without this change it would be formatted as
[GlobalObjectKey<State<StatefulWidget>> int#0000] because in Dart 2
types are instantiated to bounds.

In addition to retaining general readability this also fixes few
tests that rely on this short string representation (see
test/widgets/global_keys_duplicated_test.dart).
@mraleph mraleph requested a review from Hixie February 8, 2018 14:52
// const GlobalObjectKey<State<StatefulWidget>>().
if (selfType == 'GlobalObjectKey<State<StatefulWidget>>') {
selfType = 'GlobalObjectKey';
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this only a temporary work-around?

Seems like we'll need something permanently for this, no?

Can we compare T instead of the reified string? That might be cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove the word temporary from my comment.

Comparison against T does not work because GlobalObjectKey can have subclasses, so if I do something like:

if (T == State) {
  selfType = 'GlobalObjectKey';
}

that would be incorrect for subclasses.

Alternative to full matching I could do:

const suffix = '<State<StatefulWidget>>'
if (selfType.endsWith(suffix)) {
  selfType = selfType.substring(0, selfType.length - suffix.length);
}

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove the word temporary from my comment.

I would just remove all the mention of Dart 1, too. We're not doing this because of what Dart 1 did, we're doing it because we want a particular effect to make the dumps less confusing and cleaner, and that's how you get the effect.

...subclasses...

Ah, yeah. Your proposed code seems reasonable. We should test for that too if we don't already (and test for the case where we do want the output to include generics).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed comments, added test. Please take another look.

@mraleph
Copy link
Member Author

mraleph commented Feb 8, 2018

Analyzer is now happy too.

@mraleph
Copy link
Member Author

mraleph commented Feb 9, 2018

@Hixie ping

@Hixie
Copy link
Contributor

Hixie commented Feb 9, 2018

LGTM

@mraleph mraleph merged commit d20125c into flutter:master Feb 9, 2018
@mraleph mraleph deleted the fix-global-key-to-string branch February 9, 2018 08:52
srawlins added a commit to srawlins/flutter that referenced this pull request Feb 9, 2018
* master: (88 commits)
  Upgrade dartdoc to 0.16.0. (flutter#14602)
  Reduce noise in xcodebuild stdout (flutter#14586)
  Add annotations to ignore analyzer errors on pre-dev.22 SDKs (flutter#14599)
  Roll engine to 337764e (flutter#14597)
  Roll engine with rolled dart (flutter#14538)
  Upgrade packages (flutter#14588)
  Change GlobalObjectKey.toString to strip away State<StatefulWidget>. (flutter#14558)
  Disable selection of the Android ARM64 target platform based on the attached device (flutter#14581)
  Add a flutter run option that can override the default target platform (flutter#14537)
  Fix message channel usage in few tests. (flutter#14557)
  Update documentation on how to test flutter_tools (flutter#14567)
  Fix text_style_text.dart to be Dart 2 compliant. (flutter#14559)
  Hide the NDK warning that should never happen with a regexp (flutter#14503)
  Fix iOS build which broke after the change to fuse --strong option into --preview-dart-2.
  Mark run_machine_concurrent_hot_reload as flaky (flutter#14563)
  Fuse --strong into --preview-dart-2 option.
  Partially fix Dart 2 issues in animated_icons.test. (flutter#14531)
  Strong mode fixes in tests (flutter#14520)
  Fix typo.
  Fix issues.
  ...
DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018
…lutter#14558)

This allows const GlobalObjectKey(0) to be concisely formatted as
[GlobalObjectKey int#0000] in both Dart 2 and Dart 1 modes.

Without this change it would be formatted as
[GlobalObjectKey<State<StatefulWidget>> int#0000] because in Dart 2
types are instantiated to bounds.

In addition to retaining general readability this also fixes few
tests that rely on this short string representation (see
test/widgets/global_keys_duplicated_test.dart).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants