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

Typst Test Fixes #391

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

grantlemons
Copy link
Collaborator

Thanks to @Andrew15-5 for suggestions on #302.

@Andrew15-5
Copy link

(Idiomatic tests would probably be best suited by you creating a new pr though)

Bu-but you still included it in this PR. How come? Did you mean other than this one idiomatic example?

@Andrew15-5
Copy link

Additionally, FYI, at least in GitHub (and GitLab), you can attribute/credit someone by adding this to the end of the comment body:


Co-authored-by: name <email>

https://stackoverflow.com/questions/7442112/how-to-attribute-a-single-commit-to-multiple-developers

@Andrew15-5
Copy link

grantlemons#3

@grantlemons
Copy link
Collaborator Author

(Idiomatic tests would probably be best suited by you creating a new pr though)

Bu-but you still included it in this PR. How come? Did you mean other than this one idiomatic example?

Yes, I meant further examples, since mine was just something I cobbled together by looking at the documentation, so even the simplified version may not include things one would actually use when writing a document.

Don't feel obligated to, I'm just inviting it if you think it would be helpful.

@Andrew15-5
Copy link

Don't feel obligated to, I'm just inviting it if you think it would be helpful.

Nah, I just want to sort out the attribution thing, since you've used my code without any credit in the commits (only in GitHub comments). Hence, #391 (comment). I also wanted to create the PR myself to directly apply my fixes. My proposal for the current state of the PR is to rebase the changes, specifically editing the commit messages of the first two commits with the attribution line I showed.

It's actually not easy for me to come up with some random examples, so I'm not sure about this. And I also feel like adding just any examples will just bloat the repo. If there are some clear guidelines, then maybe I can cook something up.

@grantlemons
Copy link
Collaborator Author

grantlemons commented Jan 16, 2025

@Andrew15-5 I've amended the commit containing your simplified document to add the Co-authored-by statement. If credit is important to you, I'm fine with letting you open your own PR if you'd like. I am just trying to be proactive in fixing the issues you highlighted.

As for coming up with examples, that's fine, I agree it's probably best not to bloat the repo.

@Andrew15-5
Copy link

Andrew15-5 commented Jan 16, 2025

Thank you. I have another change I would like to make: grantlemons#4. Since the intent was to just simplify the existing example (and keep the same output), I not only fixed some issues with the original one (so that everything works as intended), but also made them (complex and simplified) output the same PDF file byte-by-byte (https://typst.app/docs/reference/model/document/#parameters-date).

One problem was that adding heading and just bold text result in different output, even though diffpdf says they look the same. This is because the heading also add some semantic changes to the PDF (apparently even if I add outlined: false). So I reverted it to being just a bold text.

@Andrew15-5
Copy link

Nice. I have one last thing to discuss, as I still not understand it completely.

Assume the wrapped paragraph in file.typ:

Lorem ipsum dolor sit amet, officia excepteur ex fugiat reprehenderit enim
labore culpa sint ad nisi Lorem pariatur mollit ex esse exercitation amet. Nisi
anim cupidatat excepteur officia. Reprehenderit nostrud nostrud ipsum Lorem est
aliquip amet voluptate voluptate dolor minim nulla est proident. Nostrud
officia pariatur ut officia. Sit irure elit esse ea nulla sunt ex occaecat
reprehenderit commodo officia dolor Lorem duis laboris cupidatat officia
voluptate. Culpa proident adipisicing id nulla nisi laboris ex in Lorem sunt
duis officia eiusmod. Aliqua reprehenderit commodo ex non excepteur duis sunt
velit enim. Voluptate laboris sint cupidatat ullamco ut ea consectetur et est
culpa et culpa duis.
Output

image

As the tests show, I assume each of the adjacent lines will be separated by the Newline(1) token. But will Harper still recognize the paragraph as a single block? Because IIUC, since there is Newline between a heading and a paragraph, and it makes the Harper to spellcheck them separately, this means that all these lines will be checked separately which is not the desired behavior. #302 (comment)

@grantlemons grantlemons changed the title Typst Fixes Typst Test Fixes Jan 17, 2025
@grantlemons
Copy link
Collaborator Author

I was wrong about newlines, they don't break up sequences. That being said, that means the translator needs to be changed to use parbreaks instead of newlines for headers &c.

We've already gotten some issues with the parser, so I'm going to put together a general typst parser fixes PR to try to get those sorted. I'll include newline vs. parbreak in that.

@Andrew15-5
Copy link

OK, so if that thing will be covered in a separate PR, then we can merge this one.

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