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

added to_json function #4229

Merged
merged 6 commits into from
Sep 4, 2024
Merged

added to_json function #4229

merged 6 commits into from
Sep 4, 2024

Conversation

aznszn
Copy link
Contributor

@aznszn aznszn commented Sep 3, 2024

added the to_json function under #4216

@aznszn
Copy link
Contributor Author

aznszn commented Sep 3, 2024

The last test I am a bit unsure about since trying to parse the returned into JSON into serde_json::Value results in an error

@aznszn aznszn changed the title added to_json function added to_json function Sep 3, 2024
@wowinter13 wowinter13 requested a review from a team September 3, 2024 11:49
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

Thanks for working on this 🙏

I added two comments about potential improvements, if you need help with the nullabilty handling just let me know.

/// let result = diesel::select(to_json::<Integer, _>(1))
/// .get_result::<Value>(connection)?;
///
/// let expected: Value = serde_json::from_str("1").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

can we use the serde_json::json! macro instead to construct the expected values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

/// # Ok(())
/// # }
/// ```
fn to_json<E: SingleValue>(e: E) -> Json;
Copy link
Member

Choose a reason for hiding this comment

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

This function seems to return null if you pass in a null values. We want to have a doc test demonstrating that this is correctly propagated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ive changed the last test a bit, let me know if its okay

Copy link
Member

Choose a reason for hiding this comment

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

No, that's not what I had in mind. My idea was to make the return type Nullable<Json> if the argument is nullable and Json for non-null arguments. As I did not know without a quick experiment how to do that I've pushed 8089dff to change it. (I needed to try it myself anyway).

To explain what's going on there:

  • The SqlType trait as a IsNull associated type that indicates whether the current type is nullable or not. We can access it as it's a super trait of SingleValue
  • The IsNull type implements MaybeNullableType<T> which wraps T into Nullable if the input type was nullable, otherwise it will just result in T.

By using that as return type we enforce that we return a non-nullable value for non-nullable inputs and nullable types otherwise, which allows us to correctly write the last test case.

@aznszn aznszn requested a review from weiznich September 4, 2024 06:27
@weiznich weiznich enabled auto-merge September 4, 2024 13:08
@weiznich weiznich added this pull request to the merge queue Sep 4, 2024
Merged via the queue into diesel-rs:master with commit f95f06d Sep 4, 2024
48 checks passed
@weiznich weiznich mentioned this pull request Sep 11, 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