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

remove the older protocol implementation #2722

Merged
merged 5 commits into from
Nov 27, 2023
Merged

remove the older protocol implementation #2722

merged 5 commits into from
Nov 27, 2023

Conversation

devoncarew
Copy link
Member

@devoncarew devoncarew commented Nov 18, 2023

  • remove the older protocol implementation (net of ~8,000 lines deleted)
  • re-organize code in lib/src for clarity
  • re-organize and update the tests (they now run in ~30s)
  • remove references to and deps on protobufs
  • update documentation

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Copy link
Contributor

@johnpryan johnpryan left a comment

Choose a reason for hiding this comment

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

Lots of good cleanup! I think the defaultChannel change might have been included by accident, but otherwise LGTM.

pkgs/dart_services/README.md Show resolved Hide resolved
# https://github.com/dart-lang/test/blob/master/pkgs/test/doc/configuration.md.

# Some of the project_creator_test.dart tests don't like to run in parallel with
# other tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this might be something we should investigate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably; I don't think it's necessary a serious underlying issue, and even w/ this the tests finish in ~30 seconds. As you say though, it would be good to investigate, esp. to rule out something serious.

// <flutter-sdk>/bin/cache/dart-sdk
final dart = Platform.resolvedExecutable;
final dartSdk = path.dirname(path.dirname(dart));
final flutterSdk = path.dirname(path.dirname(path.dirname(dartSdk)));
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) We might want to consider making sdkPath a field on the Sdk class. Right now we are caching the value globally, and then the Sdk constructor uses this value to get the Dart executable path back out again.

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 took a stab at that in this PR

pkgs/sketch_pad/lib/model.dart Outdated Show resolved Hide resolved
@devoncarew devoncarew merged commit 768d702 into main Nov 27, 2023
11 checks passed
@devoncarew devoncarew deleted the cleanup branch November 27, 2023 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants