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

expr_as(): accept and return Self by value #1979

Closed
wants to merge 1 commit into from

Conversation

Expurple
Copy link
Contributor

I was writing a complex select query and was annoyed by ownership errors forcing me to add noisy .to_owned() after every .expr_as():

some::Entity::find()
    .select_only()
    .expr_as(..., ...)
    .to_owned()
    .column(...)
    .expr_as(..., ...)
    .to_owned()
    .column_as(..., ...)
    ...

This didn't look typical for the sea-orm API, so I dug into it, and it's clearly an accident. All other methods in that file accept self by value, except for query() accessor. That's the convention.

So I went and changed expr_as() to do that as well. It's a small change that may not be backwards-compatible in theory (couldn't find any info on that), but it didn't break any tests or any of my queries in practice. The old code still compiles as is, but can now be also shortened to

some::Entity::find()
    .select_only()
    .expr_as(..., ...)
    .column(...)
    .expr_as(..., ...)
    .column_as(..., ...)
    ...

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 22, 2023

Thank you for catching this! But you're also right, this is a breaking change, which we shouldn't land right now. Here's what I can do: create a new method for now, and publish a minor release, so you can use it meanwhile.

tyt2y3 added a commit that referenced this pull request Nov 22, 2023
@tyt2y3
Copy link
Member

tyt2y3 commented Nov 22, 2023

@tyt2y3 tyt2y3 closed this Nov 22, 2023
@Expurple
Copy link
Contributor Author

LGTM, thanks. So, you gather some new features for 0.13.0, go through FIXMEs before the release, and land the breaking change in 0.13.0?

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 23, 2023

Yup! that's the plan. I would batch several breaking changes in one release, so that at least the annoyance is once off.

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.

None yet

2 participants