Skip to content

Conversation

@johnmccutchan
Copy link
Contributor

  • Don't use package:stack_trace.
  • Don't use Chain.capture.
  • Fix an instance of aggressive catching of exceptions

Related #8742

@tvolkert

@tvolkert
Copy link
Contributor

Looks like this fails on Windows

@johnmccutchan
Copy link
Contributor Author

@goderbauer PTAL at the Windows specific fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

/cc @yjbanov who had occasion to change this code to work this way just last week.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, n/m -- he didn't add the Chain.capture(), and this is still semantically the same

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a slight semantic difference here -- previously, it would use the value of traceStartup that was passed to its constructor if it was unspecified in run(). Now it'll always default to false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@tvolkert
Copy link
Contributor

LGTM with 1 comment

- [x] Don't use package:stack_trace.
- [x] Don't use Chain.capture.
- [x] Fix an instance of aggressive catching of exceptions

Related #8742
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

Should we also remove the dependency in pubspec.yaml?

@johnmccutchan johnmccutchan merged commit 4c91b6e into flutter:master Mar 15, 2017
@Hixie
Copy link
Contributor

Hixie commented Mar 16, 2017

+1 to this patch

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 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.

5 participants