[Enhancement] Nullsafety #153

j4qfrost · 2021-02-25T10:23:45Z

Other than the given nullsafety breaking change, I've changed the parameters for the the client to take the port number as an optional parameter.

isoos · 2021-02-26T06:23:45Z

Thank you, I'll find some time to review this soon.

isoos

First check looks really good, I have only a few small worries and requests.
Please also write a CHANGELOG entry for this change.

lib/src/connection.dart

isoos · 2021-02-27T22:41:36Z

lib/src/connection.dart

@@ -214,7 +215,7 @@ class PostgreSQLConnection extends Object
  /// default query timeout will be used.
  Future transaction(
    Future Function(PostgreSQLExecutionContext connection) queryBlock, {
-    int commitTimeoutInSeconds,
+    int commitTimeoutInSeconds = 30,


I'd rather keep these optional parameters as nullable and set a default value just before we use them.

isoos · 2021-02-27T22:42:15Z

lib/src/connection.dart

-    Map<String, dynamic> substitutionValues,
-    bool allowReuse,
-    int timeoutInSeconds,
+    Map<String, dynamic> substitutionValues = const {},


I'd rather keep these optional parameters as nullable and set a default value just before we use them.

isoos · 2021-02-27T22:44:03Z

lib/src/connection.dart

-  Future<int> execute(String fmtString,
-      {Map<String, dynamic> substitutionValues, int timeoutInSeconds}) async {
+  Future<int?> execute(String fmtString,
+      {Map<String, dynamic> substitutionValues = const {},


Same here: I'd rather keep these optional parameters as nullable and set a default value just before we use them. Especially because it is important to be consistent about it, and there timeoutInSeconds is nullable.

isoos · 2021-02-27T22:47:57Z

lib/src/connection_fsm.dart

      if (returningException != null) {
-        query.completeError(returningException);
+        if (message.state == ReadyForQueryMessage.StateTransactionError) {


How sure are you that message.state == ReadyForQueryMessage.StateTransactionError and returningException == null is never true at the same time? Maybe this should handle it as the original code?

That was a bit of a mistake on my part, I nested message.state == ReadyForQueryMessage.StateTransactionError within message is ReadyForQueryMessage, because the first statement did a type check that made the second statement safe. Nesting it inside the returningException != null if statement was overstepping.

I think I may be misremembering my reasoning here.

j4qfrost · 2021-02-28T00:35:20Z

@isoos

First check looks really good, I have only a few small worries and requests.
Please also write a CHANGELOG entry for this change.

Should I bump the version up too? Seems like a major release.

isoos

Should I bump the version up too? Seems like a major release.

There is no functional or otherwise breaking change, so this could be 2.3.0. The SDK constraint makes it clear for clients on 2.10 that they won't be able to use it. We can then do a 3.0.0 with the proposed change of the port property and maybe other parts can have adjustments...

isoos · 2021-02-28T10:08:47Z

lib/src/connection.dart

-    bool allowReuse,
-    int timeoutInSeconds,
+    Map<String, dynamic>? substitutionValues,
+    bool allowReuse = true,


Please remove all of these default values. It makes it much easier to develop a tool like package:postgres_pool

isoos · 2021-02-28T10:09:24Z

lib/src/connection.dart

+    Map<String, dynamic>? substitutionValues,
+    required bool allowReuse,
+    int? timeoutInSeconds,
+    bool resolveOids = true,


it could be explicitly required too.

isoos · 2021-02-28T10:09:39Z

lib/src/connection.dart

-      bool allowReuse,
-      int timeoutInSeconds}) async {
+      {Map<String, dynamic> substitutionValues = const {},
+      bool allowReuse = false,


same here: please remove defaults

j4qfrost · 2021-02-28T10:17:38Z

It does break for apps that use earlier versions of the flutter sdk though, right? Maybe a prerelease would work? Or a 2.3.0-nullsafety?

…

On Feb 28, 2021, at 2:14 AM, István Soós ***@***.***> wrote: @isoos commented on this pull request. Should I bump the version up too? Seems like a major release. There is no functional or otherwise breaking change, so this could be 2.3.0. The SDK constraint makes it clear for clients on 2.10 that they won't be able to use it. We can then do a 3.0.0 with the proposed change of the port property and maybe other parts can have adjustments... In lib/src/connection.dart <#153 (comment)>: > String fmtString, { - Map<String, dynamic> substitutionValues, - bool allowReuse, - int timeoutInSeconds, + Map<String, dynamic>? substitutionValues, + bool allowReuse = true, Please remove all of these default values. It makes it much easier to develop a tool like package:postgres_pool In lib/src/connection.dart <#153 (comment)>: > String fmtString, { - Map<String, dynamic> substitutionValues, - bool allowReuse, - int timeoutInSeconds, - bool resolveOids, + Map<String, dynamic>? substitutionValues, + required bool allowReuse, + int? timeoutInSeconds, + bool resolveOids = true, it could be explicitly required too. In lib/src/connection.dart <#153 (comment)>: > String fmtString, - {Map<String, dynamic> substitutionValues, - bool allowReuse, - int timeoutInSeconds}) async { + {Map<String, dynamic> substitutionValues = const {}, + bool allowReuse = false, same here: please remove defaults — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#153 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6S7P3CWRE56LVNHZLFFELTBIJQTANCNFSM4YGHD6JQ>.

isoos · 2021-02-28T10:20:26Z

It does break for apps that use earlier versions of the flutter sdk though, right?

I do not see how it would break them. As far as I know Flutter respects the Dart SDK constraints too.

Maybe a prerelease would work? Or a 2.3.0-nullsafety?

I don't see it making much difference, it can work too.

j4qfrost · 2021-02-28T10:29:42Z

I’m still a bit new to nullsafety migration. My experiences have involved being yelled at pub get for conflicting dependencies. Just realized it’s the responsibility of the end user to upgrade the required sdk version to use the null safety features anyway. I need to check the naming standards for nullsafety. Now that I think about it -nullsafety might just be a sort of buzzword trick to get people to use the library. Does everything look ok outside of the versioning?

…

On Feb 28, 2021, at 2:20 AM, István Soós ***@***.***> wrote: It does break for apps that use earlier versions of the flutter sdk though, right? I do not see how it would break them. As far as I know Flutter respects the Dart SDK constraints too. Maybe a prerelease would work? Or a 2.3.0-nullsafety? I don't see it making much difference, it can work too. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#153 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6S7PYXCPYND65LWUXMQGTTBIKHNANCNFSM4YGHD6JQ>.

isoos · 2021-02-28T10:34:16Z

Does everything look ok outside of the versioning?

There are still pending default values in the public API that should be rather reverted, like int timeoutInSeconds = 30,. Otherwise I think it looks good.

hpoul · 2021-02-28T10:37:24Z

Regarding version bump, this night be of interest: dart-lang/sdk#41398 (comment)

(The -nullsafety prerelease tag was only recommended while pre-beta, nowadays there so no need for that any more)

j4qfrost · 2021-02-28T10:37:35Z

Yoops, I got it. Pushed.

…

On Feb 28, 2021, at 2:34 AM, István Soós ***@***.***> wrote: Does everything look ok outside of the versioning? There are still pending default values in the public API that should be rather reverted, like int timeoutInSeconds = 30,. Otherwise I think it looks good. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#153 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA6S7PYYWYUERQLNED5SV5TTBIL3HANCNFSM4YGHD6JQ>.

isoos · 2021-03-13T22:53:14Z

This has been adopted in my soft-fork repository and published as 2.3.0-null-safety.0.

j4qfrost added 21 commits February 22, 2021 19:32

initial migration

c62f501

update travis

ec523df

update sdk requirement

1ebb99f

change error type from dynamic to Object

850f475

update tests and set longer timeout

ee5eb44

nullable column description

57d6e64

allow null key

603fa18

change to empty stacktrace

3e37c29

revert stacktrace

16700fd

query null result

c17db31

nullable execute

e0fa64f

nullable future

be96021

oops forgot

2affed0

nullable tablename

daaedfe

overwritten reuse bool error

430ceb1

encoding empty string

e8026d8

encoding empty string

17bac0c

more encoding decoding stuff

0a981d7

linting

62381eb

consistency

39e4461

own worst enemy

9d0851a

This was referenced Feb 25, 2021

Support for nullsafety stablekernel/aqueduct#917

Open

Support for null-safety #152

Open

isoos requested changes Feb 27, 2021

View reviewed changes

j4qfrost added 2 commits February 27, 2021 16:27

change params to nullable and revert port

e741fa0

revert port on tests

473f4df

j4qfrost force-pushed the master branch from 72ff687 to 473f4df Compare February 28, 2021 00:46

revert and change incrementally

d4b155e

j4qfrost added 4 commits February 27, 2021 17:04

nullable sub values

3fe22bd

ReadyForQueryMessage transaction

45b2873

forgot to save

72db712

nullable default substitute

3a45f6c

j4qfrost force-pushed the master branch from ad2fdff to 3a45f6c Compare February 28, 2021 01:38

isoos reviewed Feb 28, 2021

View reviewed changes

changelog bump version

45fd4bc

oops forgot

fd31392

j4qfrost added 3 commits March 6, 2021 23:10

nullable param dataTypeStringForDataType

2232839

change not caught before inheritance is weird

515ad44

change not caught before inheritance is weird

15026ca

isoos closed this Mar 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhancement] Nullsafety #153

[Enhancement] Nullsafety #153

j4qfrost commented Feb 25, 2021

isoos commented Feb 26, 2021

isoos left a comment

isoos Feb 27, 2021

isoos Feb 27, 2021

isoos Feb 27, 2021

isoos Feb 27, 2021

j4qfrost Feb 28, 2021

j4qfrost Feb 28, 2021

j4qfrost commented Feb 28, 2021

isoos left a comment

isoos Feb 28, 2021

isoos Feb 28, 2021

isoos Feb 28, 2021

j4qfrost commented Feb 28, 2021 via email

isoos commented Feb 28, 2021

j4qfrost commented Feb 28, 2021 via email

isoos commented Feb 28, 2021

hpoul commented Feb 28, 2021

j4qfrost commented Feb 28, 2021 via email

isoos commented Mar 13, 2021

[Enhancement] Nullsafety #153

[Enhancement] Nullsafety #153

Conversation

j4qfrost commented Feb 25, 2021

isoos commented Feb 26, 2021

isoos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j4qfrost commented Feb 28, 2021

isoos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j4qfrost commented Feb 28, 2021 via email

isoos commented Feb 28, 2021

j4qfrost commented Feb 28, 2021 via email

isoos commented Feb 28, 2021

hpoul commented Feb 28, 2021

j4qfrost commented Feb 28, 2021 via email

isoos commented Mar 13, 2021