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 comment for pgtype.Interval struct #2113

Merged
merged 1 commit into from
Aug 26, 2024
Merged

Conversation

mateuszkowalke
Copy link
Contributor

Comment explicitly states that pgtype.Interval is a nullable postgres type.

@mateuszkowalke mateuszkowalke changed the title Add comment for pgtype.Interval struct Add comment for pgtype.Interval struct Closes #2111 Aug 22, 2024
@mateuszkowalke mateuszkowalke changed the title Add comment for pgtype.Interval struct Closes #2111 Add comment for pgtype.Interval struct Aug 22, 2024
Copy link
Owner

@jackc jackc left a comment

Choose a reason for hiding this comment

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

Sorry, but there are several errors in the comments.

But beyond that, I really don't think that this is the place to explain Valid. Otherwise, we would need more or less duplicate comments on all the types. A package level comment on the pgtype package would be more reasonable.

@@ -26,6 +26,21 @@ type IntervalValuer interface {
IntervalValue() (Interval, error)
}

// Interval represents an interval that may be null.
// Interval implements the [Scanner] interface so
// it can be used as a scan destination:
Copy link
Owner

Choose a reason for hiding this comment

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

Implementing Scanner is not why it can be used as a destination in pgx. That's only when used through database/sql.

// // NULL value
// }
//
// When using as a parameter for prepared statement
Copy link
Owner

Choose a reason for hiding this comment

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

Whether or not a prepared statement is used makes no difference.

// }
//
// When using as a parameter for prepared statement
// the [Valid] field has to be explicitly set to [true] by the user.
Copy link
Owner

Choose a reason for hiding this comment

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

Valid has to be set if it is not NULL. It is perfectly reasonable for Valid to be false when representing a NULL.

Additional information warns about using nullable types being
used as parameters to query with Valid set to false.
@mateuszkowalke
Copy link
Contributor Author

Many thanks for your comments. Made me have a deeper look into pgtype. I added a short notice to the doc.go.
Anyway I get your point about the behavior of pgtype types being similar to database/sql and why it might be a bit redundant to add another explanation in pgtype, so feel free to simply reject changes.

Anyhow, wanted to thank you for your awesome work!

@jackc jackc merged commit 603f233 into jackc:master Aug 26, 2024
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.

2 participants