Skip to content

Conversation

@nonl4331
Copy link
Contributor

@nonl4331 nonl4331 commented Oct 6, 2025

Includes a version bump to 1.0.7.

PR now allows all of the following to work properly.

        LineComment(Assign[a][1], [Initialize $a$ to 1])
        LineComment([text], [Initialize $a$ to 1])
        LineComment(Break, [Initialize $a$ to 1])
        LineComment((1,), [Initialize $a$ to 1])
        LineComment((), [Initialize $a$ to 1])

@nonl4331
Copy link
Contributor Author

nonl4331 commented Oct 6, 2025

Example of how the above renders (without the LineComment((), [Initialize $a$ to 1]) which renders as ()). See issue #27 for more detail.
image

@quachpas
Copy link
Collaborator

quachpas commented Oct 6, 2025

Thank you!
Please remove the version bump, I'll take a look

@nonl4331
Copy link
Contributor Author

nonl4331 commented Oct 6, 2025

Thank you! Please remove the version bump, I'll take a look

Done. I didn't mention this above, but expressions such as LineComment($a > b$, [Initialize $a$ to 1]) work as well.

@quachpas
Copy link
Collaborator

quachpas commented Oct 6, 2025

Great, thank you @nonl4331. Can you add a corresponding test and mention it in the changelog? Thank you!

@nonl4331
Copy link
Contributor Author

nonl4331 commented Oct 7, 2025

Great, thank you @nonl4331. Can you add a corresponding test and mention it in the changelog? Thank you!

I've modified the existing linecomment test to include the new linecomment modes. I decided to do it all in a single test since:

  1. There were only a few statements and all of which were LineComment
  2. To ensure that there is a way to detect bugs and regressions when multiple LineComments are present (such as an invalid parser state caused by a correctly rendered statement which would otherwise go unnoticed)

I'm not sure what you mean by the changelog, the commit for the test is descriptive and I could not find anything in the repo unless you mean NEWS.md?

I did not change the LineComment documentation in the README.md as the new uses align with what is already documented implicitly.

Is there anything I missed or further changes that are actionable by me that are needed?

@quachpas
Copy link
Collaborator

quachpas commented Oct 7, 2025

Thank you again, yes I meant the NEWS.MD

Everything in a single test is fine! I'll test then merge, that should be everything needed

@nonl4331
Copy link
Contributor Author

nonl4331 commented Oct 7, 2025

Thank you again, yes I meant the NEWS.MD

Everything in a single test is fine! I'll test then merge, that should be everything needed

Did you want me to put something in NEWS.md or will you being doing that? If I am to put something in NEWS.md which version would it come under?

@quachpas
Copy link
Collaborator

quachpas commented Oct 7, 2025

If you can put something in news that's be great, just put it under the next patch version

@nonl4331
Copy link
Contributor Author

nonl4331 commented Oct 8, 2025

If you can put something in news that's be great, just put it under the next patch version

I've added an entry under 1.0.7 in NEWS.md. Note that I have not bumped the version from 1.0.6 to 1.0.7. It is up to you when you would like that to happen.

Apologies for the late response, it seems I have an issue with my github notifications that I will need to resolve.

@patrick-kidger
Copy link

patrick-kidger commented Oct 10, 2025

FYI I think there's still an issue here, for IfElseChain:

#import "@preview/algorithmic:1.0.6": *

// Line comment definition from this PR
#let LC(l, c) = {
  if type(l) == array and l.len() > 0 { 
    ([#l.first()#h(1fr)#CommentInline(c)], ..l.slice(1)) 
  } else {
    ([#l#h(1fr)#CommentInline(c)],)
  }
}

#algorithm({
    // This works
    LC(Line[], [bar1])

    // This doesn't
    LC(IfElseChain([baz], {Line[]}), [bar2])
})

@quachpas
Copy link
Collaborator

@nonl4331 could you add this in your test? Probably needs arraify

@nonl4331
Copy link
Contributor Author

Thanks, I'll look into a fix and add a test. I'll amend my PR over the next couple of days or so.

@nonl4331
Copy link
Contributor Author

I've pushed an update that should fix the problem @patrick-kidger, thanks for pointing it out. The test now covers the following:

  LineComment(Assign[a][1], [Initialize $a$ to 1])
  LineComment([Initialize hashmap], [Count instances])
  LineComment(Break, [Early exit])
  LineComment([$c += a-b$], [Sum of differences])
  LineComment(IfElseChain([condition], {Line[Test]}), [It's just amazing])
  LineComment([], [blank])
  LineComment(While([stuff], {Line[Another test]}), [foo])
  LineComment(Function("add", ($a$, $b$), {Assign[c][$a+b$]}), [bar])
  LineComment(LineComment([nested], [inner]), [outer])

Thanks @quachpas for the arraify tip. It wasn't the problem in this case but it did allow me to simply the LineComment code ontop of the fix. I simply had to flatten the array passed in this case.
image

@quachpas
Copy link
Collaborator

Thank you @nonl4331 , I'll take a look when I have time. Unfortunately I'm quite busy at the moment so don't worry if this does not get any review within the month.

@quachpas quachpas self-requested a review October 25, 2025 02:25
Copy link
Collaborator

@quachpas quachpas left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@quachpas quachpas merged commit 64d4d14 into typst-community:main Nov 11, 2025
1 check failed
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.

3 participants