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

[WIP] configurable encoding + optimizing json access #132

Closed
wants to merge 3 commits into from
Closed

Conversation

isoos
Copy link
Owner

@isoos isoos commented Sep 17, 2023

No description provided.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 82.85% and project coverage change: -0.05% ⚠️

Comparison is base (7bfe932) 86.51% compared to head (715caa7) 86.47%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
- Coverage   86.51%   86.47%   -0.05%     
==========================================
  Files          29       30       +1     
  Lines        2492     2521      +29     
==========================================
+ Hits         2156     2180      +24     
- Misses        336      341       +5     
Files Changed Coverage Δ
lib/src/text_codec.dart 80.70% <0.00%> (ø)
lib/src/v3/types.dart 67.64% <0.00%> (-9.78%) ⬇️
lib/src/logical_replication_messages.dart 71.42% <72.41%> (ø)
lib/src/v3/connection.dart 85.10% <75.00%> (-0.21%) ⬇️
lib/src/v3/protocol.dart 85.71% <88.88%> (ø)
lib/src/binary_codec.dart 97.54% <94.11%> (+0.02%) ⬆️
lib/postgres_v3_experimental.dart 51.16% <100.00%> (ø)
lib/src/client_encoding.dart 100.00% <100.00%> (ø)
lib/src/connection.dart 93.59% <100.00%> (ø)
lib/src/message_window.dart 100.00% <100.00%> (ø)
... and 2 more

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

@isoos
Copy link
Owner Author

isoos commented Sep 17, 2023

/cc @simolus3 @insinfo

I was trying to adapt the optimization from #130 on the encoding PR from @insinfo, but it seems to be a huge drag on the codebase, and it will increase the boilerplate required for many objects (incl. server and client messages that were not all covered on the encoding PR).

I'm open for suggestions, however, if there is no better option, I shall complete this PR with the missing parts (utf8_encoded_string, and the configuration settings on both connection)...

@simolus3
Copy link
Contributor

I don't see a much better way to do this either. Maybe we could put the message serialization / deserialization logic into a single class and turn the existing classes into fairly dumb data classes. That might reduce some of the duplicate parameters in the message layer. But I'm not sure if this actually simplifies things or just moves the problem around.

@isoos
Copy link
Owner Author

isoos commented Sep 23, 2023

Closing this as I think a refactoring of the message classes is required before we can introduce encoding efficiently.

@isoos isoos closed this Sep 23, 2023
@isoos isoos deleted the encoding branch September 23, 2023 10:49
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