Corrected the regex to detect espaced percent symbol.

[KhaledSamir]

This PR fixes #138921. The fix is correcting the regex that was being used for inline comment.

"%" should comment an inline comment but "%" should escape and treat the text if you write % along with a text after it. Example:

$$ % This is comment $$
Rendered as empty space.

$$ % Show this text $$
Rendered as "%Showthistext"

How to test

• You can refer to this file to see how VS renders it: /vscode/extensions/vscode-colorize-tests/test/colorize-fixtures/md-math.md
• You can create your own .md file and test the cases.

[ghost]

All CLA requirements met.

[Nopec]

Hey @KhaledSamir,

I think your PR looks nice. But there is one thing I have to mention. Maybe you already considered the following, in that case just ignore me, I'm also pretty new to this repository 😊. Eventually there is a more experienced contributor who might clarify that or posts a link to a resource where the vscode tests are explained.

In my research I figured out that this file extensions/vscode-colorize-tests/test/colorize-results/md-math_md.json
contains the expected test results for the integration tests. Did you execute them?

When you are running the integration tests it takes your changes here extensions/vscode-colorize-tests/test/colorize-fixtures/md-math.md and generates the vscode editor output somehow and checks the output against the expected values in here extensions/vscode-colorize-tests/test/colorize-results/md-math_md.json

If you change the behavior in highlighting you can simply run the integration tests and check if other highlighting is affected or not.

After having no failures, just add your changes here extensions/vscode-colorize-tests/test/colorize-fixtures/md-math.md and run the integration test again. The tests will fail and automatically alter the vscode-colorize-tests/test/colorize-results/md-math_md.json file. Now you can run the tests again and they should pass and you can commit the changes.

Long story short, I've this gut feeling that your changes break the integration tests. I'll checkout your PR tomorrow morning and check if the integration test are still running.

P.S.: To execute the integration tests. You can do bash scripts/test-integration.sh in your devcontainer.

[KhaledSamir]

@Nopec Very good point, Andre! I actually didn't run the integration tests but I tests my changes in all default themes and it worked fine. But of course, this doesn't mean to avoid running the integration tests, so I tried to run them but I always get this error when I do:

This error happens with test-integration or test.sh files. Are you getting the same or is it working for you?

[Nopec]

@KhaledSamir No, I didn't encounter your problem.

I made a new devcontainer using your forked repository, checked out the corresponding branch ksaleh-katexcomment. Next I executed yarn install and ran the integration tests by executing bash scripts/test-integration.sh.

As I expect I'm getting this error:

I copied the whole output to this file:
integration-test-fails-log-19-12-21.txt

After failing I get these unstaged changes:
https://diffy.org/diff/a729be820ce1d

Feel free to download this patch, apply it to your branch and amend the commit.

1. git checkout ksaleh-katexcomment
2. git apply downloaded-diff-file.diff
3. git add extensions/vscode-colorize-tests/test/colorize-results/md-math_md.json
4. git commit --amend
5. Save and exit
6. git push --force-with-lease

If you did so, I will checkout your changes and try again to execute the integration tests.
I hope we didn't miss something else. 🚀

[Nopec]

You also have to adjust your PR description, you need to write \\% to get \%. Now your text is a little bit confusing.

[Nopec]

Hello, I saw that my diff link was expired. So, here is a permanent one: https://diffy.org/diff/a729be820ce1d
I updated the link in my previous answer as well, to prevent confusion in the future.

@KhaledSamir You have any updates regarding fixing the integration tests? If you struggle, I could create a pull request to your fork if you want. Then we'll see what happens. Just, let me know.

[KhaledSamir]

Hi @Nopec,

Thank you so much for helping with the PR, am actually travelling now and would be hard for me to do that until Friday or maybe tomorrow. Could you please help with a PR to my fork repo?

[Nopec]

@KhaledSamir sure, I'll do so tomorrow. Stay safe!

[Nopec]

@KhaledSamir Unfortunately, I'm not able to create such a PR. You might give me access to your fork, so I'd be able to extend your PR. On the other hand, I think it is not a problem to wait until Friday or even longer. Have a wonderful Christmas! 🎄

[KhaledSamir]

@Nopec Unfortunately, still am getting the same error "dirname path" but I gave you access to my fork. Could you please update the file and ran the integration tests again?

[Nopec]

@KhaledSamir I updated the test expectation values and rebased this branch to the latest main. The scripts/test-integration.sh script runs successfully. We should wait now for further reviews. Nice. 🚀

[KhaledSamir]

@Nopec Thank you, Andre! :) I'll try to fix this issue that I'm facing whenever I ran the integration tests script. Let's wait for the reviews now 💯

[mjbvz]

Thanks! The change looks good to me. Will be in the next insiders build and is scheduled for VS Code 1.64