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

[flutter_tools] Separate style and data from AnalysisError #60591

Merged
merged 8 commits into from Jul 9, 2020
Merged

[flutter_tools] Separate style and data from AnalysisError #60591

merged 8 commits into from Jul 9, 2020

Conversation

figureConan
Copy link
Contributor

Extract error data structure and wrap it with command line styles according to the platform, terminal, fileSystem.

Description

#49589 commit reduce context usage in analyze command and tests, and also introduce new variables(platform, terminal, fileSystem).These new variables affect output styles in different environments.
I suggest to extract error information data structure from the AnalysisError class. So we can use WrittenError for plain text content. And wrap WrittenError with AnalysisError.

Related Issues

Replace this paragraph with a list of issues related to this PR from our issue database. Indicate, which of these issues are resolved or fixed by this PR. There should be at least one issue listed here.

Tests

I added the following tests:

Reuse flutter analyze command test case.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

Extract error data structure and wrap it with command line styles according to the platform, terminal, fileSystem.
@fluttergithubbot
Copy link
Contributor

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 dev. 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.

@fluttergithubbot fluttergithubbot changed the base branch from dev to master June 30, 2020 11:12
@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jun 30, 2020
@fluttergithubbot
Copy link
Contributor

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.

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

1 similar comment
@fluttergithubbot
Copy link
Contributor

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.

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

@figureConan figureConan changed the title Separate style and data from AnalysisError [flutter_tools] Separate style and data from AnalysisError Jun 30, 2020
@jonahwilliams jonahwilliams self-requested a review July 2, 2020 17:10
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

It would be nice to add some unit test coverage to WrittenError. The existing tests are mostly integration and don't really cover the small cases like JSON serialization well.

packages/flutter_tools/lib/src/dart/analysis.dart Outdated Show resolved Hide resolved
packages/flutter_tools/lib/src/dart/analysis.dart Outdated Show resolved Hide resolved
packages/flutter_tools/lib/src/dart/analysis.dart Outdated Show resolved Hide resolved
packages/flutter_tools/lib/src/dart/analysis.dart Outdated Show resolved Hide resolved
@figureConan
Copy link
Contributor Author

It would be nice to add some unit test coverage to WrittenError. The existing tests are mostly integration and don't really cover the small cases like JSON serialization well.

Thank you williams for reviewing, all fixed.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

Except for the small change in the test

@jonahwilliams
Copy link
Member

@IncludeCmath this will land automatically once the build is green, all good to go here 👍

@figureConan
Copy link
Contributor Author

figureConan commented Jul 9, 2020

Google testing is pending @jonahwilliams

@fluttergithubbot fluttergithubbot merged commit 72258ea into flutter:master Jul 9, 2020
@figureConan figureConan deleted the feature-extract-error branch July 10, 2020 02:38
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

None yet

4 participants