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

Make PgLTree::push infallible and take PgLTreeLabel directly. #1734

Merged
merged 3 commits into from
Apr 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 10 additions & 7 deletions sqlx-core/src/postgres/types/ltree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,17 @@ pub enum PgLTreeParseError {
pub struct PgLTreeLabel(String);

impl PgLTreeLabel {
pub fn new(label: &str) -> Result<Self, PgLTreeParseError> {
pub fn new<S>(label: S) -> Result<Self, PgLTreeParseError>
where
String: From<S>,
{
let label = String::from(label);
if label.len() <= 256
&& label
.bytes()
.all(|c| c.is_ascii_alphabetic() || c.is_ascii_digit() || c == b'_')
{
Ok(Self(label.to_owned()))
Ok(Self(label))
} else {
Err(PgLTreeParseError::InvalidLtreeLabel)
}
Expand Down Expand Up @@ -101,20 +105,19 @@ impl PgLTree {
/// creates ltree from an iterator with checking labels
pub fn from_iter<I, S>(labels: I) -> Result<Self, PgLTreeParseError>
where
S: Into<String>,
String: From<S>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you're calling parse() on it, we don't need to copy it to an owned string. I would make this S: AsRef<str> and then do label.as_ref().parse() in the for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need owned Strings inside the PgLTreeLabel anyways, I changed PgLTreeLabel::new to take S: Into<String> and changed the call in here from parse()? to PgLTreeLabel::new.

This saves copies when the caller doesn't need to own the string but leaves &str valid!

I: IntoIterator<Item = S>,
{
let mut ltree = Self::default();
for label in labels {
ltree.push(&label.into())?;
ltree.push(PgLTreeLabel::new(label)?);
}
Ok(ltree)
}

/// push a label to ltree
pub fn push(&mut self, label: &str) -> Result<(), PgLTreeParseError> {
self.labels.push(PgLTreeLabel::new(label)?);
Ok(())
pub fn push(&mut self, label: PgLTreeLabel) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unfortunately a breaking change, though, and will need to wait for 0.6.0. I don't have an exact timeline on that right now but I expect it to be the next release we do, probably in a month or so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the info! The work-around is not too terrible anyways

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the info! The work-around is not too terrible anyways

You could add a new push_ function with the new signature and leave the existing one as-is.

self.labels.push(label);
}

/// pop a label from ltree
Expand Down