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

Add builtin time, geometric and most range types #294

Merged
merged 8 commits into from
Feb 17, 2024
Merged

Add builtin time, geometric and most range types #294

merged 8 commits into from
Feb 17, 2024

Conversation

wolframm
Copy link
Contributor

@wolframm wolframm commented Feb 15, 2024

I added some builtin types as discussed in issue #293.

This is the first time I contribute to a project on Github, so I'm more than happy to fix any issues you might see with pull request.

Once is merged, I would add numrange, but I wanted to ask if it wouldn't be better to change the current numeric type from GenericType<Object>(TypeOid.numeric) that accepts int, double and String to GenericType<Decimal>(TypeOid.numeric) using the decimal library (https://pub.dev/packages/decimal).

@isoos
Copy link
Owner

isoos commented Feb 15, 2024

Thanks for the PR! I'll have time for review in the coming days, but a quick look:

  • code analysis CI test is failing, please check the lints/warnings/errors and fix them
  • please bump the version and add a few bits about the PR in CHANGELOG.md
  • It is easier to review/iterate on smaller PRs, e.g. one or just a few related type / PR. If it is not that hard to split, I would be very happy to review more smaller PRs than a large one. (If not no worries, it just make take longer).

@wolframm
Copy link
Contributor Author

wolframm commented Feb 16, 2024

I ran dart test and fixed one failing test case for daterange. Sorry about that.

I bumped the version and added a note to changelog.

I ran dart analyze and dart test and no issues were found on Dart version 3.2.0, so hopefully everything should work now.

The most tricky part of the code were the range types. I think I covered most edge cases in encoding_test.dart, but this would be the best place to try to "brake" my code. Geotypes and time seemed rather simple.

Next time I will add things in smaller chunks.

@wolframm
Copy link
Contributor Author

wolframm commented Feb 16, 2024

I just upgraded my Dart version to 3.3.0 and now I also saw the warnings that your CI flow reported. I fixed the warning that was in the code that I contributed, but the following warning is not part of my work, as I made no changes to server_messages.dart:

warning • lib/src/messages/server_messages.dart:412:17 • The parameter type of '==' operators should be non-nullable. Try using a non-nullable type. • non_nullable_equals_parameter

Could you make the necessary fix in server_messages.dart (or ignore the warning for now)?

@isoos
Copy link
Owner

isoos commented Feb 16, 2024

Could you make the necessary fix in server_messages.dart (or ignore the warning for now)?

Good catch! Pushed a fix to the git repo, you may rebase to it (or just adapt the same change).

Copy link
Owner

@isoos isoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome! I have mostly nits and a few rename requests, plus a few clarification questions.

CHANGELOG.md Outdated Show resolved Hide resolved
lib/postgres.dart Outdated Show resolved Hide resolved
lib/src/types.dart Outdated Show resolved Hide resolved
lib/src/types.dart Outdated Show resolved Hide resolved
lib/src/types/binary_codec.dart Show resolved Hide resolved
lib/src/types/range_types.dart Outdated Show resolved Hide resolved
lib/src/types/type_registry.dart Outdated Show resolved Hide resolved
lib/src/types/type_registry.dart Outdated Show resolved Hide resolved
lib/src/types/type_registry.dart Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

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

Comparison is base (eff50c6) 85.75% compared to head (4758951) 85.68%.
Report is 1 commits behind head on master.

Files Patch % Lines
lib/src/types/geo_types.dart 65.43% 28 Missing ⚠️
lib/src/types/range_types.dart 85.29% 15 Missing ⚠️
lib/src/types.dart 48.14% 14 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #294      +/-   ##
==========================================
- Coverage   85.75%   85.68%   -0.08%     
==========================================
  Files          25       27       +2     
  Lines        2261     2662     +401     
==========================================
+ Hits         1939     2281     +342     
- Misses        322      381      +59     

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

@wolframm
Copy link
Contributor Author

Is there anything you want me to do regarding the Codecov report?

All the issues flagged regarding the ==-operator should be covered by expectReversible function in encoding_test.dart. Or perhaps I misunderstand what 'expectReversible` is doing.

@isoos
Copy link
Owner

isoos commented Feb 17, 2024

Thanks for the updates! I'll merge this now, and play with it locally, maybe refactor a bit here and there. I am planning to publish the new version with this later today or maybe tomorrow at latest.

@isoos isoos merged commit 8cee7fa into isoos:master Feb 17, 2024
1 check passed
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