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

Don't escape colons when writing verbatim fields #22

Merged
merged 2 commits into from
May 3, 2022

Conversation

mutlusun
Copy link
Contributor

@mutlusun mutlusun commented May 2, 2022

Hello,

this PR fixes the discussion around colons in #21 .

I'm not sure whether it's smartest way to differentiate between a write and read mode but didn't come up with a better idea so far. Looking forward to your comments!

Best

Copy link
Member

@reknih reknih left a comment

Choose a reason for hiding this comment

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

This PR looks good and like a good way to maximize compatibility. I only have a little nit about the condition. After that, we can merge it.

src/resolve.rs Outdated
'&' | '%' | '$' | '_' if !verb => true,
':' if read_char => true,
Copy link
Member

Choose a reason for hiding this comment

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

The colon would not be escaped when writing with this condition, even if the field is not to be dumped in verbatim mode. Instead, I suggest the following:

Suggested change
':' if read_char => true,
':' if read_char || !verb => true,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion! I added this.

One question: Is there a specific reason for escaping all colons (in non-verbatim mode)? So far, I thought colons are not a problem in bibtex/biblatex files.

However, I found the following issue: JabRef/jabref#5982 If I understand it correctly, colons are only a problem in the bibtex key.

Thanks for your clarification and your help.

Copy link
Member

Choose a reason for hiding this comment

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

Oops I overlooked your comment. You are right, as far as I see, the colon need not be escaped in values but only in keys. This means that the code would have been right after all, without my suggestion. As we've seen, there is no harm in being permissive about what escapes to accept when reading. If this is no urgent matter I'll revert back to your original PR in the next bugfix release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback! No, this isn't a urgent matter. 😄

Escape colon also in non-verbatim mode

Co-authored-by: Martin Haug <mhaug@live.de>
@reknih reknih merged commit b58244c into typst:main May 3, 2022
reknih added a commit that referenced this pull request May 12, 2022
Also won't escape colon in verbatim outputs (follow up on #22)
Bumps version
reknih added a commit that referenced this pull request May 12, 2022
Also won't escape colon in verbatim outputs (follow up on #22)
Bumps version
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