Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes async gap handling. #84

Merged
merged 2 commits into from Oct 8, 2020
Merged

Fixes async gap handling. #84

merged 2 commits into from Oct 8, 2020

Conversation

ghost
Copy link

@ghost ghost commented Oct 7, 2020

This fixes an issue where an async gap at the end of a stack trace would not get parsed correctly due to the trailing newline being trim()'d.

A couple of tests were added to cover this case.

@google-cla google-cla bot added the cla: yes label Oct 7, 2020
@ghost ghost requested a review from natebosch October 7, 2020 15:06
Copy link
Member

@natebosch natebosch left a comment

Choose a reason for hiding this comment

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

Can you add a CHANGELOG entry?

lib/src/trace.dart Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Oct 8, 2020

RE:CHANGELOG: I assume I'm supposed to bump the version and update the pubspec too, right?

@natebosch
Copy link
Member

I assume I'm supposed to bump the version and update the pubspec too, right?

In this case no, the existing version was OK. Anytime there is a trailing -dev on a version it should mean we are in between publishing, and can keep using the same version. When we get ready to publish we drop the -dev.

https://github.com/dart-lang/sdk/wiki/External-Package-Maintenance#making-a-change

For future PRs it would also help to avoid a force push. We do a squash and merge once the PR is ready so we don't need to clean up into a single commit and force push ahead of time.

https://github.com/dart-lang/sdk/wiki/External-Package-Maintenance#handling-pull-requests

@natebosch natebosch merged commit 45319bf into dart-lang:master Oct 8, 2020
@mkustermann
Copy link
Member

@natebosch We are a little pressed on time. Could you help us get this change published on pub and pulled into flutter/flutter? (we can do the latter)

@natebosch
Copy link
Member

I opened #85

Once that is approved I'll publish and update here.

@natebosch
Copy link
Member

This is published as 1.10.0-nullsafety.3

@ghost
Copy link
Author

ghost commented Oct 14, 2020

Thanks, Nate.

I see 1.10.0-nullsafety.3 on pub, but I noticed it has an odd [UNIDENTIFIED] tag on it, and the log points to this being an SDK version issue.

In the commit message you mentioned bumping the SDK version, but would it be possible to only bump the patch version so we can stay within currently stable SDK versions?

The reason I'm asking is that flutter update-packages --force-upgrade doesn't seem to pick up 1.10.0-nullsafety.3, and I suspect it's to do with the SDK version.

@natebosch
Copy link
Member

You are trying to update the version pinned in the Flutter SDK? I would not have expected the SDK version to be a problem since they should be on 2.11.0-dev SDKs by now...

In any case I tested this on earlier SDKs and can't find an incompatibility so I'll drop the min back down.
#86

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants