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.

Sign up for GitHub

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

Jump to bottom

Add Bool type to MultiConnection #3747

Merged
merged 1 commit into from
Aug 16, 2023
Merged

Add Bool type to MultiConnection #3747

merged 1 commit into from
Aug 16, 2023

Conversation

BlackDex
Copy link
Contributor

During trying to implement MultiConnection i found that the Bool type was missing.

This PR adds this type.

@BlackDex
Add Bool type to MultiConnection
8d2075a 
During trying to implement MultiConnection i found that the `Bool` type
was missing.

This PR adds this type.
@weiznich weiznich requested a review from a team August 15, 2023 14:53
@weiznich
Copy link
Member

I think just adding this impl would be a breaking change :(

Existing users of #[derive(MultiConnection)] could have already manually implemented that these traits. On the other hand "breaking" changes are allowed for bug fixes and we could argue that this is a bug fix.
(I wouldn't expect any breakage here as otherwise we would have seen reports about the missing impl earlier)

@BlackDex
Copy link
Contributor Author

I would also say this is a bug fix, since this is a missing default type. It's not that this is a whole new type not previously existing on the separate backends.

@weiznich
Copy link
Member

cc @diesel-rs/core on whether or no we want to consider that as bug fix.

@pksunkara
Copy link
Member

I am not core but I would consider this a bug fix.

@sgrif
Copy link
Member

sgrif commented Aug 15, 2023

This seems backwards compatible afaict so treating it as a bug fix seems fine

@Ten0
Copy link
Member

Ten0 commented Aug 15, 2023

IMO if somebody was competent enough to realize that impl was missing and implemented it manually without bothering to open an issue, breaking change is fair enough ^^

@weiznich
Copy link
Member

Thanks for the swift responses. Seems like we are all on line in calling this a bug fix 👍

@pksunkara Your input is appreciated here as well ❤️

@weiznich weiznich added this pull request to the merge queue Aug 16, 2023
Merged via the queue into diesel-rs:master with commit 02a37df Aug 16, 2023
@BlackDex BlackDex mentioned this pull request Aug 16, 2023
Closed
3 tasks
@BlackDex BlackDex deleted the add-bool-type-to-multiconnection branch August 16, 2023 13:05
weiznich added a commit to weiznich/diesel that referenced this pull request Aug 16, 2023
@weiznich
Fix diesel-rs#3748
77c906a 
We now also generate the necessary `FromSql`/`ToSql` impls for
`chrono`/`time` types for `Time`/`Date`/`Timestamp`. This follows the
same reasoning as #diesel-rs#3747.
@weiznich weiznich mentioned this pull request Aug 16, 2023
Merged
weiznich added a commit to weiznich/diesel that referenced this pull request Aug 16, 2023
@weiznich
Fix diesel-rs#3748
c0d5b8c 
We now also generate the necessary `FromSql`/`ToSql` impls for
`chrono`/`time` types for `Time`/`Date`/`Timestamp`. This follows the
same reasoning as #diesel-rs#3747.
weiznich added a commit to weiznich/diesel that referenced this pull request Aug 17, 2023
@weiznich
Fix diesel-rs#3748
8d2497a 
We now also generate the necessary `FromSql`/`ToSql` impls for
`chrono`/`time` types for `Time`/`Date`/`Timestamp`. This follows the
same reasoning as #diesel-rs#3747.
weiznich added a commit to weiznich/diesel that referenced this pull request Aug 18, 2023
@weiznich
Fix diesel-rs#3748
6bc5233 
We now also generate the necessary `FromSql`/`ToSql` impls for
`chrono`/`time` types for `Time`/`Date`/`Timestamp`. This follows the
same reasoning as #diesel-rs#3747.
weiznich added a commit to weiznich/diesel that referenced this pull request Aug 18, 2023
@weiznich
Fix diesel-rs#3748
1d66cdf 
We now also generate the necessary `FromSql`/`ToSql` impls for
`chrono`/`time` types for `Time`/`Date`/`Timestamp`. This follows the
same reasoning as #diesel-rs#3747.
weiznich added a commit to weiznich/diesel that referenced this pull request Aug 18, 2023
@weiznich
Merge pull request diesel-rs#3747 from BlackDex/add-bool-type-to-mult…
9056ab3 
…iconnection

Add `Bool` type to MultiConnection
@weiznich weiznich mentioned this pull request Aug 18, 2023
Merged
@weiznich weiznich mentioned this pull request Sep 8, 2023
Open
@BrewTestBot BrewTestBot mentioned this pull request May 31, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ten0 Ten0 Ten0 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@BlackDex @weiznich @pksunkara @sgrif @Ten0