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

Use v3 as delegate behind v2 interface. #149

Merged
merged 1 commit into from
Oct 10, 2023
Merged

Use v3 as delegate behind v2 interface. #149

merged 1 commit into from
Oct 10, 2023

Conversation

isoos
Copy link
Owner

@isoos isoos commented Oct 8, 2023

Use the following to trigger v3 as delegate:

V3=1 dart test test/decode_test.dart

@isoos
Copy link
Owner Author

isoos commented Oct 8, 2023

/cc @simolus3

I think this may be useful for many things:

  • applying the tests before migrating, making sure that we don't miss anything,
  • a reviewable implicit guide for migration
  • an optional interface to use for old code waiting for migration

wdyt?

@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2023

Codecov Report

Attention: 73 lines in your changes are missing coverage. Please review.

Comparison is base (d022423) 87.40% compared to head (e448c5d) 84.95%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage   87.40%   84.95%   -2.45%     
==========================================
  Files          29       30       +1     
  Lines        2532     2605      +73     
==========================================
  Hits         2213     2213              
- Misses        319      392      +73     
Files Coverage Δ
lib/src/v2_v3_delegate.dart 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@simolus3
Copy link
Contributor

simolus3 commented Oct 9, 2023

I think this is great to increase confidence in the v3 implementation by running it against unchanged tests, but some tests (like the "you don't need to await statements and it's completely fine" tests) are also testing implementation quirks. I don't really see much use in a perfect compatibility layer (which would likely be very hard to write anyway) when v3 is not just a new implementation, but hopefully also something we consider to be an improved API.

I think we'll likely get most existing tests to pass with a compatibility layer written with a couple hours of effort (and that's worth it IMO), but getting every existing test to pass with this approach will be much harder.
I'm happy to contribute to this wrapper to get more of the existing tests to pass, but I also don't want to do much work twice. For instance, I've already spent a considerable amount of time migrating and debugging the transaction tests.

an optional interface to use for old code waiting for migration

Couldn't code waiting to be migrated just continue using version 2 of this package? If we continue exposing v2 APIs in an eventual 3.0.0 release under say package:postgres/legacy.dart, we'd have to continue supporting that until a v4 release. Is that maintenance burden worth it?

@simolus3
Copy link
Contributor

simolus3 commented Oct 9, 2023

I've extended the delegate a bit, but running the first tests against it already reveals things like trying to read private fields with reflection to capture internal state. I really don't see much value in the delegate when there will be a considerable effort required to migrate tests either way.

With the tests I've migrated so far, they turned out to reveal actual issues in the new implementation which was very helpful. But so far, the experience of testing against a delegate has been pretty frustrating so I think it will be easier to migrate tests and then get a migration guide from the diff.

@isoos
Copy link
Owner Author

isoos commented Oct 10, 2023

getting every existing test to pass with this approach will be much harder.

I think we should improve the interface for the 95% of the tests, and mark non-trivial tests with skip: isV3Wrapper, and review them case-by-case, if the feature (or its test) is to drop. I think it increases the confidence for ourselves and for others too.

Couldn't code waiting to be migrated just continue using version 2 of this package? If we continue exposing v2 APIs in an eventual 3.0.0 release under say package:postgres/legacy.dart, we'd have to continue supporting that until a v4 release. Is that maintenance burden worth it?

I was thinking an explicit break in 3.1.0. E.g. 3.0.0 has both, but old one in let's call it legacy.dart, with a Deprecated marker that it will be removed in 3.1.0. This allows a somewhat gradual migration for larger codebases.

@isoos isoos merged commit 7c75c92 into master Oct 10, 2023
1 check passed
@isoos isoos deleted the delegate branch October 10, 2023 19:37
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.

3 participants