Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Low-level interop for PG arrays #636
Low-level interop for PG arrays #636
Changes from 17 commits
e8650f7
39566c0
98196df
4eea77c
9cffc1b
48176a2
82790eb
d67dbf0
431e89e
a60a6dd
dfcfbb6
e5fc2f5
48ccccb
11c5a63
d74719c
6900356
bbad726
133b44d
59dac25
0da6f1c
7bfa2e4
55f62ac
b4a7364
f763c0a
b0b3477
639d0b4
55deefe
1785c07
16cf849
e2a3509
9979f77
f15eabd
b2c9508
4ba1b0d
5ab4ba8
fd3c21d
1dd9155
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you returning
Option<RawArray>
when this panics on nullptr?https://github.com/tcdi/pgx/blob/7163fe2437f8305b57e461697005d48de3526bbb/pgx/src/datum/array.rs#L95-L98
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to change it to not do that NULL-check in a followup PR. Should I fix that now? It feels vaguely off to diff outside this addition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nah that's fine, just leave this unresolved until the followup PR, it's nonblocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this path be visible in the public documentation? I suppose they can click through to see the source, but the public docs will support links.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, actually, directly linking is a good idea. And that way we could also link to the doxygen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doxygen is too unstable, will point to master and lines might change. Instead used gitweb links.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a pgx-maintainers perspective, how much of a maintenance nightmare is it linking to the doxygen pages? How often would that change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially every time someone lands a commit into postgresql/master.