Skip to content

impl FromSql for various heap-allocated string and blob slices#1558

Merged
gwenn merged 3 commits intorusqlite:masterfrom
H2CO3:master
Aug 27, 2024
Merged

impl FromSql for various heap-allocated string and blob slices#1558
gwenn merged 3 commits intorusqlite:masterfrom
H2CO3:master

Conversation

@H2CO3
Copy link
Copy Markdown
Contributor

@H2CO3 H2CO3 commented Aug 26, 2024

Just what it says on the tin. This adds impl FromSql for:

  • Cow<'_, T> where T: ToOwned + FromSql
  • Rc<str>
  • Arc<str>
  • Box<[u8]>
  • Rc<[u8]>
  • Arc<[u8]>

Unit tests are included and passing. This doesn't seem to break anything used by the current test suite, and adding concrete impls shouldn't usually be a breaking change, so it seems we're good to go.

@gwenn
Copy link
Copy Markdown
Collaborator

gwenn commented Aug 26, 2024

I don't konw why postgres doesn't support these types:
https://docs.rs/postgres-types/latest/postgres_types/trait.FromSql.html
?

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.95%. Comparing base (19a4c55) to head (af1befe).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1558      +/-   ##
==========================================
+ Coverage   85.81%   85.95%   +0.14%     
==========================================
  Files          57       57              
  Lines       10664    10725      +61     
==========================================
+ Hits         9151     9219      +68     
+ Misses       1513     1506       -7     

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

@H2CO3
Copy link
Copy Markdown
Contributor Author

H2CO3 commented Aug 26, 2024

I don't konw why postgres doesn't support these types: https://docs.rs/postgres-types/latest/postgres_types/trait.FromSql.html ?

No idea, to be honest. Seems like an omission. They don't seem to break anything, and I can't think of anything obvious why they shouldn't be supported, either. (And I can give good arguments as to why they should.)

@gwenn
Copy link
Copy Markdown
Collaborator

gwenn commented Aug 26, 2024

@thomcc Would you mind giving your review ?

Copy link
Copy Markdown
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

Looks like a good change to me.

@H2CO3
Copy link
Copy Markdown
Contributor Author

H2CO3 commented Aug 27, 2024

@thomcc @gwenn Sorry, I have zero idea how to please rustfmt (I don't use it). Last time it failed because lines were too long, now it fails because they are too short. 😭 Can you help me format the code correctly? I don't think I'll be able to figure it out in a reasonable time, given that every change apparently requires a manual re-run on your part.

@thomcc
Copy link
Copy Markdown
Member

thomcc commented Aug 27, 2024

You should just run cargo fmt in the repository, and it will format it correctly for you.

@H2CO3
Copy link
Copy Markdown
Contributor Author

H2CO3 commented Aug 27, 2024

You should just run cargo fmt in the repository, and it will format it correctly for you.

Thanks, will do this soon.

@H2CO3
Copy link
Copy Markdown
Contributor Author

H2CO3 commented Aug 27, 2024

@thomcc Done, albeit the formatting of the getter closures became very… peculiar, to put it mildly.

@gwenn gwenn merged commit 4ab7b27 into rusqlite:master Aug 27, 2024
@gwenn
Copy link
Copy Markdown
Collaborator

gwenn commented Aug 27, 2024

Thanks!

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