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

Fix delimiting within <choose> and <layout> for CSL #269

Merged
merged 6 commits into from
Feb 1, 2025

Conversation

YDX-2147483647
Copy link
Contributor

@YDX-2147483647 YDX-2147483647 commented Jan 22, 2025

There is a design trade-off:
The delimiter stack uses String instead of &str.
Using &str could eliminate many cloning, but at the cost of introducing complex lifetimes.
Considering that delimiters are typically short, I choose String.

Besides, I bump some *.cbor to pass tests. If this is not desired, I can reset the commit.

Fixes typst#180

A design trade-off:
The delimiter stack uses `String` instead of `&str`.
Using `&str` could eliminate many cloning, but at the cost of introducing complex lifetimes.
Considering that delimiters are typically short, I choose `String`.
@YDX-2147483647 YDX-2147483647 marked this pull request as ready for review January 22, 2025 16:58
@YDX-2147483647 YDX-2147483647 mentioned this pull request Jan 22, 2025
YDX-2147483647 added a commit to YDX-2147483647/citationberg that referenced this pull request Jan 22, 2025
Copy link
Contributor

@PgBiel PgBiel left a comment

Choose a reason for hiding this comment

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

Thank you!

@PgBiel PgBiel merged commit ec8cdeb into typst:main Feb 1, 2025
2 checks passed
@YDX-2147483647
Copy link
Contributor Author

You are welcome!

And I notice that you updated the doc comments for push/pop (7a341cc).

(the delimiter) is inherited by [citationberg::Choose] for its children as well.

The world might be a bit confusing. In XML, the term_child_ only includes the direct ones. (The term for both direct and indirect ones is descendant.)
The spec uses output instead:

delimiting the output of the child elements

applied within the output of cs:choose

@PgBiel
Copy link
Contributor

PgBiel commented Feb 1, 2025

It is correct in the sense that it only passes the delimiter forward to children, which may decide to pass it further down the tree or not - usually not, as the point of delimiter is to delimit only the immediate children of a group or layout, unless the child of the choose is another choose. The usage of the word "output" seems to be there more to indicate that it won't apply to, say, the <else> branch if it wasn't matched, but otherwise it will apply to the final children.

So I guess it's more a matter of how you interpret it - it's true that the delimiter may affect descendants other than children with nested <choose>, but I also wasn't looking to summarize how the children handle delimiters.

Either way, we could adopt a wording closer to the spec to be sure.

@YDX-2147483647 YDX-2147483647 deleted the choose-del branch February 1, 2025 18:27
reknih pushed a commit to typst/citationberg that referenced this pull request Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants