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

TypelevelSiteSettings - deprecate top level color properties in favor of a nested object #692

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

jenshalm
Copy link
Contributor

@jenshalm jenshalm commented Apr 6, 2024

Previously the color properties were all individual top level properties, drowning out the most relevant API elements. Secondly, their naming reflected the actual color, not their usage, which means that any changes in look & feel would lead to the need for new deprecations to avoid misleading names.

This PR moves all top level color properties into a new nested object colors and renames them so that they reflect their usage. The existing properties are all deprecated.

A possible change I would recommend, but did not do in this first version of the PR in case you want to be very conservative about it: I checked within the Typelevel organization, and not a single color property is used anywhere. So there is an option to remove these properties right away. In case you'd prefer that, too, I'll update the PR.

@mergify mergify bot added the site label Apr 6, 2024
@j-mie6
Copy link
Collaborator

j-mie6 commented Apr 9, 2024

I'd definitely prefer it if the original colour names were kept as comments on the definition sites: that way it's easier to remember what they actually look like.

I'd also be in favour of keeping them as private variables, alternatively.

@jenshalm
Copy link
Contributor Author

jenshalm commented Apr 10, 2024

I'd definitely prefer it if the original colour names were kept as comments on the definition sites: that way it's easier to remember what they actually look like.

I'd also be in favour of keeping them as private variables, alternatively.

Sure, we can add comments, we can make the colors object private, we can even do both (e.g. comments are for maintainers). I really don't mind either way, so you can just make a call here.

What about the deprecated properties themselves? Do you want to keep them for now despite nobody using them?

@armanbilge
Copy link
Member

Do you want to keep them for now despite nobody using them?

@j-mie6 I think you added them in #573, was there a reason you made them public?

I lean towards deprecation before removing esp. if it's not creating maintenance burden. But if there really is little use for them then I guess it would be fine.

@j-mie6
Copy link
Collaborator

j-mie6 commented Apr 10, 2024

Just in case anybody wanted to use them... I'm happy for them to be privatised. If nobody is using them, doesn't seem like a big deal

@jenshalm
Copy link
Contributor Author

I pushed a commit that makes the colors object private. There are pros and cons for this and it seems you lean towards making them private.

I also kept the deprecated properties and just adjusted the deprecation messages. I believe nobody would notice if we'd delete them right away, but at the same time the maintenance cost of keeping them is so minimal that it's also fine. Just remember to delete them for 0.8.

@j-mie6
Copy link
Collaborator

j-mie6 commented Apr 11, 2024

Nobody is using them that we know of, so I'm totally in favour of just removing instead of deprecating. We're just a build system after all, and this is a major bump

@jenshalm
Copy link
Contributor Author

Nobody is using them that we know of, so I'm totally in favour of just removing instead of deprecating. We're just a build system after all, and this is a major bump

I agree, but I thought @armanbilge was favouring to keep them?

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks both, this is fine for now. If we receive feedback then we can revisit visibility.

the color properties were all individual top level properties, drowning out the most relevant API elements

This is definitely sub-optimal. Something we can consider doing in the 0.7.x series after sufficient deprecation period is making them source-private but still public binary.

@armanbilge armanbilge merged commit d66d4be into typelevel:main Apr 11, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants