Skip to content

Conversation

brianquinlan
Copy link
Collaborator

@brianquinlan brianquinlan commented Jun 13, 2024

There was some confusion about the semantics raised in #422


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

@brianquinlan brianquinlan requested a review from natebosch June 13, 2024 23:29
@brianquinlan brianquinlan changed the title Add a note saying that we only creating a single Client. Add a note saying that we only create a single Client. Jun 13, 2024
@natebosch
Copy link
Member

Can you expand this? Maybe add a bit more detail in the comment in source, and elaborate further in the CL description if necessary.

With the current comment, my first assumption was that it means http_factory.httpClient() returns a singleton instance and I was confused by the naming.

I think what it actually means is Provider<Client> is a singleton provider, and it only calls it's create argument once. Is that correct?
I don't know anything about Provider, and I don't think we should assume the reader does either. If we're describing some property of that library I think we should make it explicit.

@brianquinlan
Copy link
Collaborator Author

Can you expand this? Maybe add a bit more detail in the comment in source, and elaborate further in the CL description if necessary.

With the current comment, my first assumption was that it means http_factory.httpClient() returns a singleton instance and I was confused by the naming.

I think what it actually means is Provider<Client> is a singleton provider, and it only calls it's create argument once. Is that correct? I don't know anything about Provider, and I don't think we should assume the reader does either. If we're describing some property of that library I think we should make it explicit.

You're right. I've updated the comment. Is it more clear now?

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.

Very clear now. Thanks!

@brianquinlan brianquinlan merged commit 4d8e7ef into dart-lang:master Jun 24, 2024
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Jun 24, 2024
…est_descriptor, test_process, tools, typed_data, webdev, yaml

Revisions updated by `dart tools/rev_sdk_deps.dart`.

csslib (https://github.com/dart-lang/csslib/compare/23c314b..b70fef2):
  b70fef2  2024-06-20  Kevin Moore  Update and fix lints, bump min SDK to Dart 3.1 (dart-archive/csslib#204)

dartdoc (https://github.com/dart-lang/dartdoc/compare/6330a13..88df88c):
  88df88c9  2024-06-21  Sam Rawlins  Remove the unnecessary Privacy mixin (dart-lang/dartdoc#3794)
  d050cb44  2024-06-20  Sam Rawlins  Make PackageGraph.allLibraries private (dart-lang/dartdoc#3792)
  7c559d41  2024-06-20  Sam Rawlins  Simplify Library.allModelElements (dart-lang/dartdoc#3793)

http (https://github.com/dart-lang/http/compare/8c325b9..4d8e7ef):
  4d8e7ef  2024-06-24  Brian Quinlan  Add a note saying that we only create a single `Client`. (dart-lang/http#1234)

pool (https://github.com/dart-lang/pool/compare/88e4636..832c5ab):
  832c5ab  2024-06-21  Kevin Moore  update lints, require Dart 3.4 (dart-archive/pool#89)
  d107269  2024-06-20  Kevin Moore  blast_repo fixes (dart-archive/pool#88)

pub_semver (https://github.com/dart-lang/pub_semver/compare/a9025f3..dfcad38):
  dfcad38  2024-06-20  Kevin Moore  bump lints and SDK dep, test wasm (dart-archive/pub_semver#105)

string_scanner (https://github.com/dart-lang/string_scanner/compare/7b37c1b..e1cab8f):
  e1cab8f  2024-06-20  Kevin Moore  update lints, require Dart 3.1 (dart-archive/string_scanner#76)

test_descriptor (https://github.com/dart-lang/test_descriptor/compare/b23d7cc..2f19400):
  2f19400  2024-06-20  Kevin Moore  update lints (dart-archive/test_descriptor#68)

test_process (https://github.com/dart-lang/test_process/compare/862eaf3..a7ca20b):
  a7ca20b  2024-06-21  Kevin Moore  update lints (dart-archive/test_process#60)

tools (https://github.com/dart-lang/tools/compare/4321aec..e660a68):
  e660a68  2024-06-24  Kevin Moore  Don't collect coverage on Windows (dart-lang/tools#276)

typed_data (https://github.com/dart-lang/typed_data/compare/d14f965..8529929):
  8529929  2024-06-21  Kevin Moore  update lints, require Dart 3.1 (dart-archive/typed_data#90)
  c90e624  2024-06-20  Kevin Moore  blast_repo fixes (dart-archive/typed_data#89)

webdev (https://github.com/dart-lang/webdev/compare/eccc7d8..c566112):
  c5661125  2024-06-24  Kevin Moore  Drop use of deprecated APIs, bump min SDK and other dependencies (dart-lang/webdev#2449)

yaml (https://github.com/dart-lang/yaml/compare/7873b3f..4cf24ca):
  4cf24ca  2024-06-20  Kevin Moore  update lints, require Dart 3.4 (dart-archive/yaml#167)
  7018ac8  2024-06-20  Kevin Moore  blast_repo fixes (dart-archive/yaml#166)

Change-Id: I04c6238514cb46818c3f59760e401d371e4d1ae4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/372841
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants