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

Add none to type union e.types.paint #5

Open
Mc-Zen opened this issue Jan 13, 2025 · 4 comments
Open

Add none to type union e.types.paint #5

Mc-Zen opened this issue Jan 13, 2025 · 4 comments
Labels
proposal Proposal to change something in the library types Related to the type system or custom types

Comments

@Mc-Zen
Copy link

Mc-Zen commented Jan 13, 2025

This is to be discussed of course but I wonder whether it makes sense to add none to the type union e.types.paint. I think virtually everywhere in the Typst standard library, the parameter fill accepts none | color | gradient | pattern/tiling.

This makes it easier to declare fields that are to be used with native primitives like box, rect, grid, etc.

Edit: Note that stroke.paint does not accept none, however.

@PgBiel
Copy link
Owner

PgBiel commented Jan 13, 2025

Yeah, stroke was my main inspiration here. I think having to write types.option(types.paint) would be fairly acceptable in such cases.

@PgBiel
Copy link
Owner

PgBiel commented Jan 13, 2025

I had a similar thought regarding content vs option(content), but ultimately it all seems to have the same pattern; we'd end up adding none to more types than we need, I think.

@Mc-Zen
Copy link
Author

Mc-Zen commented Jan 14, 2025

Yeah, stroke was my main inspiration here. I think having to write types.option(types.paint) would be fairly acceptable in such cases.

Okay, fair point, although I think that it's the more common case that a fill also accepts none.

I had a similar thought regarding content vs option(content), but ultimately it all seems to have the same pattern; we'd end up adding none to more types than we need, I think.

However, none implicitly casts to content in Typst in most cases, right? As an example grid, highlight and all other functions that take content also take none (although not explicitly annotated in the docs). I feel like none is just some valid content.

@PgBiel PgBiel added the proposal Proposal to change something in the library label Jan 14, 2025
@PgBiel
Copy link
Owner

PgBiel commented Jan 14, 2025

Taking a look at the Typst source code (at https://github.com/typst/typst/blob/a4ac4e656267e718a5cf60d1e959f74b2b7346f3/crates/typst-library/src/foundations/value.rs#L658 ), that seems to be true, so we could do that change for parity, though I'm still a bit reluctant, albeit users could still use exact(content) if needed I suppose. I'll keep the issue open for now to gather more opinions before making a decision.

@PgBiel PgBiel added the types Related to the type system or custom types label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Proposal to change something in the library types Related to the type system or custom types
Projects
None yet
Development

No branches or pull requests

2 participants