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 LaTeX bug + add tests #96

Merged
merged 1 commit into from
Dec 16, 2021
Merged

Fix LaTeX bug + add tests #96

merged 1 commit into from
Dec 16, 2021

Conversation

qwinters
Copy link
Contributor

@qwinters qwinters commented Dec 9, 2021

This PR aims to

I was able to identify this by adding a local change to swap \begin => \\begin and \end => \\end in the regex parts for LaTeX before parsing. However, not all tests pass after doing this (which appears to be due to other underlying issues with the tests).

Also worth noting that the tests right now depend explicitly on the version of syntax.json which is on master of this repo, which makes it painful to test locally. I overrode that in the tests file to make them easier to work with in the future

@qwinters qwinters marked this pull request as ready for review December 9, 2021 20:05
@qwinters qwinters changed the title Improve LaTeX coverage Fix LaTeX bug + add tests Dec 9, 2021
@qwinters
Copy link
Contributor Author

qwinters commented Dec 9, 2021

@alstr It doesn't look like I can add you as a reviewer on this, but just wanted to comment to make sure you're aware of it. Cheers!

@alstr
Copy link
Owner

alstr commented Dec 10, 2021

Thanks very much for the work on this, I'll take a look as soon as I'm able!

@alstr
Copy link
Owner

alstr commented Dec 10, 2021

And yes, the tests should not depend on the remote syntax.json file, good observation. In general I want to improve the coverage of the tests and ideally add a test each time a new language is added.

@qwinters
Copy link
Contributor Author

qwinters commented Dec 13, 2021

@alstr - I just added tests for Julia / AutoHotKey / Org Mode / Handlebars, which are the ones listed as outstanding.

Worth noting that the Julia test is failing because it creates 3 issues instead of 2, which I think has something to do with the actual parsing logic, but I don't have bandwidth to dig into it.

I propose that if the tests work for you that we merge this to fix the LaTeX issue (which is actively blocking some of my personal workflows) and then move the Julia parsing error to a new issue / PR. Does that sound good to you?

1. Swap `\` for `\\` for TeX block comments to avoid issues with parsing
2. Add tests for TeX, Julia, AutoHotKey, Org mode, and Handlebars
3. Ensure tests can be run with 1 line command
4. Change logic in test framework so that tests run off of local version
   of `syntax.json` rather than remote version
5. Add details for running tests + adding tests for your syntax PR to
   readme
6. Add .gitignore file for python (to avoid compiles form tests winding
   up in commits)
index 0000000..7cccc5b
--- /dev/null
+++ b/src/tests/example_file.jl
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alstr FYI this is the diff for the julia failing test (the add/remove diffs are the same actual julia content).

Not quite sure why this would be parsed into 3 todos.

Copy link
Owner

Choose a reason for hiding this comment

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

I think it's because the line comment marker # appears within the block comment marker #=/=#. I haven't tested this but I've opened issue #102. Probably need to only match a marker if it is an exact match.

@qwinters
Copy link
Contributor Author

@alstr friendly ping on this! This PR is defo far from perfect, but want to get at the very least the TeX portion fixed sooner rather than later if possible. Thanks!

@alstr alstr merged commit fa33680 into alstr:master Dec 16, 2021
@alstr
Copy link
Owner

alstr commented Dec 16, 2021

Thanks very much!

@github-actions github-actions bot mentioned this pull request Jan 20, 2022
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