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

Conversation

sebpuetz
Copy link
Contributor

@sebpuetz sebpuetz commented Mar 3, 2022

Previously the function took strings and parsed them into PgLTreeLabel internally, now it's possible to directlry push PgLTreeLabels onto a PgLTree.

It was a bit awkward that pop and push returned different types

@@ -101,20 +101,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!

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.

@abonander abonander added this to the 0.6.0 milestone Mar 3, 2022
pub fn new(label: &str) -> Result<Self, PgLTreeParseError> {
pub fn new<S>(label: S) -> Result<Self, PgLTreeParseError>
where
S: Into<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this change too can be seen as a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this can break type inference, so it's breaking.

It shouldn't matter since the push change is breaking anyways and pushes this patch to the next breaking release

sebpuetz and others added 3 commits April 14, 2022 15:43
Previously the function took strings and parsed them into
PgLTreeLabel internally, now it's possible to directlry push
PgLTreeLabels onto a PgLTree.
@abonander abonander merged commit babd353 into launchbadge:master Apr 14, 2022
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