Skip to content

Conversation

Huliiiiii
Copy link
Member

#897 (comment)

I'm considering removing Expr::new, since we already have Expr::from.
As @Expurple mentioned here, I checked my codebase and found that using Expr::from makes the intent clearer. Moreover, accepting Into in the new method is not idiomatic in Rust.

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 2, 2025

I think we should leave it here for compatiblity , but we can deprecate it.

@Expurple
Copy link
Member

Expurple commented Aug 2, 2025

Agree with @tyt2y3. I'd include only the most necessary (most beneficial and unblocking) breaking changes in 1.0.0. It's already a big release. Similar to the other minor warts, I'd deprecate Expr::new later in 1.x and remove it in 2.0.0.

It would be nice to track these future deprecations somewhere. So far, I've usually put the word "legacy" somewhere in the doc-comment. Which is good to have anyway, so that no one wonders why the wart is there and whether it actually has any hidden meaning/intention

@Huliiiiii
Copy link
Member Author

Huliiiiii commented Aug 2, 2025

But this was added in 1.0.0-rc.1 (#889). I think this does not affect compatibility.

Copy link
Member

@Expurple Expurple left a comment

Choose a reason for hiding this comment

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

Ah, forgot about that

@tyt2y3 tyt2y3 merged commit 95a2072 into SeaQL:master Aug 15, 2025
20 checks passed
@Huliiiiii Huliiiiii deleted the remove-expr-new branch August 15, 2025 11:10
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