search

stablekernel / postgresql-dart

Dart PostgreSQL driver: supports extended query format, binary protocol and statement reuse.
https://www.dartdocs.org/documentation/postgres/latest
BSD 3-Clause "New" or "Revised" License
129 stars 32 forks source link

[Enhancement] Nullsafety #153

Closed j4qfrost closed 3 years ago

j4qfrost commented 3 years ago

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 commented 3 years ago

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

j4qfrost commented 3 years ago

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

j4qfrost commented 3 years ago

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 notifications@github.com 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 https://github.com/stablekernel/postgresql-dart/pull/153#discussion_r584271276:

 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 https://github.com/stablekernel/postgresql-dart/pull/153#discussion_r584271330:

 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 https://github.com/stablekernel/postgresql-dart/pull/153#discussion_r584271383:

   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 https://github.com/stablekernel/postgresql-dart/pull/153#pullrequestreview-600241773, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6S7P3CWRE56LVNHZLFFELTBIJQTANCNFSM4YGHD6JQ.

isoos commented 3 years ago

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 commented 3 years ago

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 notifications@github.com 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 https://github.com/stablekernel/postgresql-dart/pull/153#issuecomment-787428961, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6S7PYXCPYND65LWUXMQGTTBIKHNANCNFSM4YGHD6JQ.

isoos commented 3 years ago

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 commented 3 years ago

Regarding version bump, this night be of interest: https://github.com/dart-lang/sdk/issues/41398#issuecomment-786217515

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

j4qfrost commented 3 years ago

Yoops, I got it. Pushed.

On Feb 28, 2021, at 2:34 AM, István Soós notifications@github.com 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 https://github.com/stablekernel/postgresql-dart/pull/153#issuecomment-787430750, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6S7PYYWYUERQLNED5SV5TTBIL3HANCNFSM4YGHD6JQ.

isoos commented 3 years ago

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