Add "base", "final" and "interface" modifiers #920

brianquinlan · 2023-05-03T19:41:56Z

Fixes #918

brianquinlan · 2023-05-03T20:18:08Z

pkgs/http/lib/retry.dart

@@ -14,7 +14,7 @@ import 'http.dart';
 /// NOTE: [RetryClient] makes a copy of the request data in order to support
 /// resending it. This can cause a lot of memory usage when sending a large
 /// [StreamedRequest].
-class RetryClient extends BaseClient {
+final class RetryClient extends BaseClient {


Interface is identical to client. There is no reason to implement or extend this.

brianquinlan · 2023-05-03T20:18:39Z

pkgs/http/lib/src/browser_client.dart

@@ -31,7 +31,7 @@ BaseClient createClient() {
 /// [BaseRequest.followRedirects], and [BaseRequest.maxRedirects] fields. It is
 /// also unable to stream requests or responses; a request will only be sent and
 /// a response will only be returned once all the data is available.
-class BrowserClient extends BaseClient {
+base class BrowserClient extends BaseClient {


Interface is identical to Client

I wonder if this should be final?

I could imagine people wanting to add methods to Client implementations. Like:

class DownloadingBrowserClient extends BrowserClient { Stream<UInt8List> downloadFile(Url) { ... } }```

If this is usable as a skeleton/base class for people who might want to extend the interface, then I'd just make it open (no modifier).
The example you give shows exactly that: You forgot to put base on DownloadingBrowserClient, and the author of it might want to allow other implementations (for testing/mocking or genuinely considers it an interface worth implementing).

So, final or open, only use base for very particular cases where extending is fine, but inheriting implementation is essential (mainly existing code which calls private members on the type).

I expect very few base modifiers, because of the transitive "must be base" requirement. It can't be used for just "don't implement this interface, because it doesn't make sense". We don't have that, sadly. (Otherwise Object would have that modifier, but it definitely cannot be base.)

brianquinlan · 2023-05-03T20:18:50Z

pkgs/http/lib/src/io_client.dart

@@ -43,7 +43,7 @@ class _ClientSocketException extends ClientException
 }

 /// A `dart:io`-based HTTP client.
-class IOClient extends BaseClient {
+base class IOClient extends BaseClient {


Interface is identical to Client

I'd make it final or open.

There is no need to require extending subclasses to be base.

brianquinlan · 2023-05-03T20:19:12Z

pkgs/http/test/io/client_test.dart

@@ -14,14 +14,14 @@ import 'package:test/test.dart';

 import '../utils.dart';

-class TestClient extends http.BaseClient {
+base class TestClient extends http.BaseClient {


Interface is identical to Client

This class is in the test directory, so no need to give it a modifier unless one is required.

brianquinlan · 2023-05-03T20:19:16Z

pkgs/http/test/io/client_test.dart

  @override
  Future<http.StreamedResponse> send(http.BaseRequest request) {
    throw UnimplementedError();
  }
 }

-class TestClient2 extends http.BaseClient {
+base class TestClient2 extends http.BaseClient {


Interface is identical to Client

Ditto. Just no modifier. It's not a public class, so we don't care what people can do with it.

brianquinlan · 2023-05-03T20:19:28Z

pkgs/http/test/io/http_test.dart

@@ -10,7 +10,7 @@ import 'package:test/test.dart';

 import '../utils.dart';

-class TestClient extends http.BaseClient {
+base class TestClient extends http.BaseClient {


Interface is identical to Client

Also just a test class, so no modifier needed.

brianquinlan · 2023-05-03T20:26:13Z

I didn't add base to BaseRequest, BaseClient and BaseResponse but someone might want to implement (e.g. for mocking) an implementor of those classes.

lrhn

The BaseClient class is documented as "This is a mixin-style class" and it can be used as a mixin today, so I'd recommend making it a mixin class.

LGTM without the bases.

(The package could generally use some clean-up. A lot of the code seems overly complicated, and could use more modern features or just be optimized better.)

lrhn · 2023-05-09T08:23:01Z

pkgs/http/lib/src/browser_client.dart

@@ -31,7 +31,7 @@ BaseClient createClient() {
 /// [BaseRequest.followRedirects], and [BaseRequest.maxRedirects] fields. It is
 /// also unable to stream requests or responses; a request will only be sent and
 /// a response will only be returned once all the data is available.
-class BrowserClient extends BaseClient {
+base class BrowserClient extends BaseClient {


If this is usable as a skeleton/base class for people who might want to extend the interface, then I'd just make it open (no modifier).
The example you give shows exactly that: You forgot to put base on DownloadingBrowserClient, and the author of it might want to allow other implementations (for testing/mocking or genuinely considers it an interface worth implementing).

So, final or open, only use base for very particular cases where extending is fine, but inheriting implementation is essential (mainly existing code which calls private members on the type).

I expect very few base modifiers, because of the transitive "must be base" requirement. It can't be used for just "don't implement this interface, because it doesn't make sense". We don't have that, sadly. (Otherwise Object would have that modifier, but it definitely cannot be base.)

lrhn · 2023-05-09T08:25:22Z

pkgs/http/lib/src/io_client.dart

@@ -43,7 +43,7 @@ class _ClientSocketException extends ClientException
 }

 /// A `dart:io`-based HTTP client.
-class IOClient extends BaseClient {
+base class IOClient extends BaseClient {


I'd make it final or open.

There is no need to require extending subclasses to be base.

lrhn · 2023-05-09T08:29:05Z

pkgs/http/test/io/client_test.dart

@@ -14,14 +14,14 @@ import 'package:test/test.dart';

 import '../utils.dart';

-class TestClient extends http.BaseClient {
+base class TestClient extends http.BaseClient {


This class is in the test directory, so no need to give it a modifier unless one is required.

lrhn · 2023-05-09T08:29:14Z

pkgs/http/test/io/http_test.dart

@@ -10,7 +10,7 @@ import 'package:test/test.dart';

 import '../utils.dart';

-class TestClient extends http.BaseClient {
+base class TestClient extends http.BaseClient {


Also just a test class, so no modifier needed.

lrhn · 2023-05-09T09:38:49Z

pkgs/http/test/io/client_test.dart

  @override
  Future<http.StreamedResponse> send(http.BaseRequest request) {
    throw UnimplementedError();
  }
 }

-class TestClient2 extends http.BaseClient {
+base class TestClient2 extends http.BaseClient {


Ditto. Just no modifier. It's not a public class, so we don't care what people can do with it.

lrhn · 2023-05-09T09:40:11Z

pkgs/http/lib/src/byte_stream.dart

@@ -7,7 +7,7 @@ import 'dart:convert';
 import 'dart:typed_data';

 /// A stream of chunks of bytes representing a single piece of data.
-class ByteStream extends StreamView<List<int>> {
+final class ByteStream extends StreamView<List<int>> {


(This class should not need to exist. It should just be extension methods on Stream<List<int>>.)

brianquinlan · 2023-05-09T17:00:59Z

The BaseClient class is documented as "This is a mixin-style class" and it can be used as a mixin today, so I'd recommend making it a mixin class.

EDIT I wrote a lot of wrong this below. TL;DR I made BaseClient mixin.

BaseClient requires that you implement a send method so I don't think that it is currently useable as a mixin. We could restructure this a bit to have a new Sendable class like:

interface Sendable {
  Future<SendResponse> send(SendRequest request);
}

mixin BaseClient on Sendable {
...
}

But I think that should be a separate change.

Please correct me if I'm wrong - I've never actually used a mixin!

Add Dart 3.0 class modifiers

66cd601

github-actions bot added the package:http label May 3, 2023

Update monorepo file

bf7acb9

github-actions bot added the type-infra A repository infrastructure change or enhancement label May 3, 2023

brianquinlan changed the title ~~zAdd Dart 3.0 class modifiers~~ Add "base", "final" and "interface" modifiers May 3, 2023

brianquinlan added 6 commits May 3, 2023 12:55

SDK version

ae659a3

Regenerate

9e0027e

Remove a bunch of class modifiers

34ba6dc

More removes

f81982c

Update retry.dart

aeff0b3

Update CHANGELOG.md

ec4ff72

brianquinlan commented May 3, 2023

View reviewed changes

brianquinlan requested review from natebosch and lrhn May 3, 2023 20:26

brianquinlan assigned natebosch and lrhn May 3, 2023

natebosch approved these changes May 3, 2023

View reviewed changes

lrhn approved these changes May 9, 2023

View reviewed changes

brianquinlan added 2 commits May 9, 2023 10:16

Review changes

2b35be0

Make BaseClient mixin

5c70885

brianquinlan merged commit 406ce74 into dart-lang:master May 9, 2023

This was referenced Jun 4, 2023

Can't mock final classes using Mockito. #956

Closed

Updated dependencies to Dart3/Flutter 3.10 compatible ones kentcb/azure_application_insights#38

Closed

techouse mentioned this pull request Jul 15, 2023

Can not extend RetryClient because it's final #991

Closed

Add "base", "final" and "interface" modifiers #920

Add "base", "final" and "interface" modifiers #920

Uh oh!

Conversation

brianquinlan commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianquinlan commented May 3, 2023

Uh oh!

lrhn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianquinlan commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

brianquinlan commented May 3, 2023 •

edited

Loading

brianquinlan commented May 9, 2023 •

edited

Loading