-
Notifications
You must be signed in to change notification settings - Fork 1
Fix/grammar #38
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/grammar #38
Conversation
…omments, bullets and some sentences are not parsed correctly
|
Maybe I'm a bit confused about the role of the new |
It's not a new Maybe we should discuss this in more detail in person? |
|
I still need to add more unit tests to this as well. |
pimotte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a cursory review with one comment
Previously the default themeStyle was 'light' and the editor would initialize the codemirror highlighting as if the theme was 'light'. This would lead to a period where the theme was incorrectly set if the user was in a dark style theme. Now we initialize the editor with the correct theme style, without waiting for a message from the host.
…r into fix/grammar
|
@pimotte As I worked on this PR as well, maybe you should be the one to review it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Two remarks:
This breaks the dark theme for me. (Works on waterproof-vscode main, breaks on waterproof-vscode main with this PR built and linked) In particular, the lemma statements are practically invisible to me.Fixed when testing with impermeable/waterproof-vscode#259
You've written unit tests, but sonar doesn't notice coverage on new code. Is the new code uncovered and is the coverage on old code? (Also, the coverage config I think is still broken in general, but I'll pick that up.)Discussed with @DikieDick, fix is in #30
We probably want to figure out the first point before merging, the second is more an FYI.
pimotte
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical issues have been addressed. I'm good with merging after considering this comment: #38 (comment)
|


Description
Refactor of the grammar.
Changes
"Obtain ", which made that "Obtain\n" would not be colored correctly.Obtain such a(n) ...Testing this PR
Go through the unit tests, see that they are run and that they have enough coverage of different parsing situations.
Moreover, use this PR in waterproof-vscode (in conjunction with impermeable/waterproof-vscode#259), open several exercise sheets and see if they get colored correctly. Play around a bit with spaces and enters to see coloring behaves well with them.