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(glow): handle inline /* ... */ comment #465

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maurice
Copy link

@maurice maurice commented Jan 27, 2025

Fixes #463

Please shout if there is anything you are unhappy with.

@nobkd
Copy link
Collaborator

nobkd commented Jan 27, 2025

Great work so far!

I added if (comment) endComment() to line 270 in glow.js, so the comment gets closed, at the end of the lines even if there is no closing end (or no "block" closing end, to be specific).

In the following example, even though no code is truncated (with the above addition), the end is still a comment, because the closing was "inline" vs the opening was "block":

Try:

/*
hello
*/ world

Do you have a simple fix idea?

(Btw. there are still the other multiline comment types, (e.g. <!-- hello --> world) that also need an inline variant.

I guess, this grows a bit in complexity...

I'm not sure, how to solve this, but you can try giving it a stab, if you'd like :)

@maurice maurice force-pushed the glow-inline-js-comment branch from e65b06c to aea229f Compare January 27, 2025 21:05
@maurice
Copy link
Author

maurice commented Jan 27, 2025

Great work so far!

Thanks for the encouragement 😊

In the following example, even though no code is truncated (with the above addition), the end is still a comment, because the closing was "inline" vs the opening was "block":

Good test case 👍

(Btw. there are still the other multiline comment types, (e.g. <!-- hello --> world) that also need an inline variant.

Yeah that's true, and we should be able to use the same technique for all really.

I'm not sure, how to solve this, but you can try giving it a stab, if you'd like :)

I've updated the PR to handle your example (but not the other multiline comment types). I've tried to be sympathetic to the original implementation, but given the assumptions that were made originally (specifically, it's a line-by-line transformation, and each line is assumed to be either a comment or not), it's a bit hacky.

I'm not expecting this to be merged as-is, but we could talk about where to go from here.

I'm a bit concerned about making more significant changes because of the ultra minimal test suite, and there are definitely some lines I can comment-out and no tests fail 😟

@maurice maurice force-pushed the glow-inline-js-comment branch 2 times, most recently from 58f3813 to 7152750 Compare January 27, 2025 21:31
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.

Glow truncates code; output incomplete
2 participants