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

PgHasArrayType for transparent types fix. #2086

Merged
merged 1 commit into from
Sep 6, 2022

Conversation

Wopple
Copy link
Contributor

@Wopple Wopple commented Sep 6, 2022

Problem: PgHasArrayType was checking the application's postgres feature
Solution: only check the library's postgres feature

This wasn't fun to debug. 😅

Problem: PgHasArrayType was checking the application's postgres feature
Solution: only check the library's postgres feature
@@ -88,15 +88,21 @@ fn expand_derive_has_sql_type_transparent(
<#ty as ::sqlx::Type<DB>>::compatible(ty)
}
}
#[automatically_derived]
#[cfg(feature = "postgres")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line was making it so my application had to have a postgres feature which was enabled. 💢

Copy link
Collaborator

Choose a reason for hiding this comment

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

Woops! Good catch.

@abonander abonander merged commit 34e2a17 into launchbadge:main Sep 6, 2022
@abonander
Copy link
Collaborator

Ah, wait. This is a breaking change, isn't it? Anyone who manually added a PgHasArrayType impl will get a breakage from this.

abonander added a commit that referenced this pull request Sep 6, 2022
@Wopple
Copy link
Contributor Author

Wopple commented Sep 6, 2022

I didn't think about that, but you are probably correct. Still, the behavior of asking the application to create its own postgres feature feels like a violation of responsibilities. The migration story is simply deleting the manual implementations, no?

@abonander
Copy link
Collaborator

That was certainly a mistake in the implementation, the intended effect was to emit the block only when the postgres feature was enabled in SQLx.

Would you say that this is an acceptable breakage (derived types gaining the PgHasArrayType impl) considering it was already documented in the changelog for 0.6.0, it just didn't take effect? https://github.com/launchbadge/sqlx/blob/main/CHANGELOG.md#breaking

@Wopple
Copy link
Contributor Author

Wopple commented Sep 6, 2022

That's a pretty philosophical question, is the promise the code, or the documentation? I want this in a release so I'm pretty biased in my answer. 😛 FWIW, I just have a random postgres feature in my application now to work around it, so if you want to delay to another major release that's okay.

@abonander
Copy link
Collaborator

Yeah, I suppose I'll just move this to another branch for now and release it with 0.7.0.

@abonander
Copy link
Collaborator

Moved commit from main to 0.7-dev.

@Wopple Wopple deleted the fix-array-type-macro branch September 7, 2022 13:44
abonander pushed a commit that referenced this pull request Sep 15, 2022
Problem: PgHasArrayType was checking the application's postgres feature
Solution: only check the library's postgres feature

Co-authored-by: Daniel Tashjian <daniel@ecomedes.com>
abonander pushed a commit that referenced this pull request Feb 18, 2023
Problem: PgHasArrayType was checking the application's postgres feature
Solution: only check the library's postgres feature

Co-authored-by: Daniel Tashjian <daniel@ecomedes.com>
abonander pushed a commit that referenced this pull request Feb 21, 2023
Problem: PgHasArrayType was checking the application's postgres feature
Solution: only check the library's postgres feature

Co-authored-by: Daniel Tashjian <daniel@ecomedes.com>
Aandreba pushed a commit to Aandreba/sqlx that referenced this pull request Mar 31, 2023
Problem: PgHasArrayType was checking the application's postgres feature
Solution: only check the library's postgres feature

Co-authored-by: Daniel Tashjian <daniel@ecomedes.com>
mrl5 added a commit to mrl5/sqlx that referenced this pull request Jul 14, 2023
caused by launchbadge#2086
closes launchbadge#2611

seems like launchbadge#2086 fixed issue described by @Wopple by making that block
of code unreachable. Not as it was stated in
> Problem: PgHasArrayType was checking the application's postgres feature
> Solution: only check the library's postgres feature

after checking https://doc.rust-lang.org/std/macro.cfg.html
> `cfg!`, unlike `#[cfg]`, does not remove any code and only evaluates
> to true or false. For example, all blocks in an if/else expression
> need to be valid when `cfg!` is used for the condition, regardless of
> what `cfg!` is evaluating.

so to my understanding it would be `true` anyway in @woople scenario
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.

2 participants