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

feat: internalize extensions to Color #386

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cweider
Copy link

@cweider cweider commented Feb 12, 2025

I encountered this reviewing code in a project importing swift-markdown-ui. The usage had no relation to Markdown. The method was used because it was there and it was useful. I’ve not encountered a real conflict, but #265 suggests that the risk of a conflict is not speculative.

The initializers init(rgba:) and init(light:dark:), introduced in #168 are very useful, but should be internal. Marked as public, these functions are liable to cause conflicts with other packages. The potential for conflict is heightened by the functions' utility and the intuitive name.

Note: this breaks the public API; it requires a Major Version change

The initializers `init(rgba:)` and `init(light:dark:)`, introduced
in gonzalezreal#168 are very useful, but should be `internal`. Marked as
`public`, these functions are liable to cause conflicts with other
packages. The potential for conflict is heightened by the functions'
utility and the intuitive name.
@cweider cweider force-pushed the internalize-extensions branch from 35c104a to 86b7b05 Compare February 12, 2025 02:23
@cweider cweider changed the title internalize extensions to Color feat: internalize extensions to Color Feb 12, 2025
@@ -16,7 +16,7 @@ extension Color {
/// - Parameters:
/// - light: The light appearance color value.
/// - dark: The dark appearance color value.
public init(light: @escaping @autoclosure () -> Color, dark: @escaping @autoclosure () -> Color) {
Copy link
Author

Choose a reason for hiding this comment

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

Aside: AppKit and UIKit are almost certainly supposed to specify that dynamicProvider needs to be Sendable (see: FB16492911 & FB16492913).

@Sendable would be good to add prior to adopting the Swift 6 runtime.

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.

1 participant