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

Update to Diesel 2.0.0-rc.0 #29

Merged
merged 6 commits into from
Jul 5, 2022
Merged

Conversation

Emulator000
Copy link
Contributor

@Emulator000 Emulator000 commented May 2, 2022

In this PR i just updated the dependency crate to Diesel 2.0.0-rc.0 and changed the TsConfiguration accordingly to the new serializer function ToSql

@Emulator000 Emulator000 closed this May 2, 2022
@Emulator000 Emulator000 changed the title Updated Diesel reference to 1.4.8 Update to Diesel 2.0.0-rc.0 May 2, 2022
@Emulator000 Emulator000 reopened this May 2, 2022
@Emulator000
Copy link
Contributor Author

@weiznich could you review this please? thanks 😄

@weiznich
Copy link
Member

weiznich commented May 3, 2022

@Emulator000 No need to ping me for that, especially not if this PR was opened less than a day before. Remember that is a free time project for me, so it could take some time till I answer to issues or other things. Especially PR reviews can take time.

@Emulator000
Copy link
Contributor Author

sorry, no pressure at all, just because I weren't able to assign the PR review to you

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 opening this PR. This looks fine beside the u32 -> i32 change for the TsConfiguration struct. I would like to hear some justification there.

src/lib.rs Outdated
#[sql_type = "Regconfig"]
pub struct TsConfiguration(pub u32);
#[diesel(sql_type = RegConfig)]
pub struct TsConfiguration(pub i32);
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment here why the type of the inner field was changed to i32?

@FL33TW00D
Copy link

FL33TW00D commented Jul 2, 2022

@Emulator000 Can you respond to the above? Would be great to have this merged in!

@Emulator000
Copy link
Contributor Author

If I remember correctly, I had a little issue with a new lifetime introduced by the changes of the new function arguments definition, anyway it should be safe to leave as is.

I'll try to recall later but I think that is not a blocking issue.

@weiznich
Copy link
Member

weiznich commented Jul 3, 2022

I'll try to recall later but I think that is not a blocking issue.

I would like to keep the old signature there to minimize breaking changes.

@Emulator000
Copy link
Contributor Author

@weiznich I reverted to u32, the only change that I can see now is the constraint to u32 instead of i32 on the to_sql and from_sql functions for TsConfiguration but should be fine.

@Emulator000 Emulator000 requested a review from weiznich July 5, 2022 08:53
@weiznich weiznich merged commit 37224cd into diesel-rs:master Jul 5, 2022
@Emulator000 Emulator000 deleted the diesel-new branch July 5, 2022 15:17
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