Skip to content

Fix github formatting #34

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

Merged
merged 1 commit into from
Jun 8, 2019
Merged

Fix github formatting #34

merged 1 commit into from
Jun 8, 2019

Conversation

PanAeon
Copy link
Contributor

@PanAeon PanAeon commented Jun 7, 2019

I removed nested character classes after which the error seems to disappear.
Removed "$schema" field from the generated json so Linguist will not issue an unknown field warning.
Also removed redundant 'special-identifier' rule which clashes with other rules.

Lightshow highlighting examples:

issue: #33

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I'm not very qualified to review the regexes but I tried the new changes on lightshow and it seems to work great, thank you for the quick fix!

Sidenote, would it be possible to compile the regexes somehow with PCRE in CI to ensure that we don't regress in the future? I suppose this kind of validation should be done by linguist 🤔

@PanAeon
Copy link
Contributor Author

PanAeon commented Jun 8, 2019

@olafurpg, yes linguist does compile all regexes before accepting a syntax file. But that won't prevent issues like this one, when incorrect pattern is technically valid. Also there might be slight differences in semantics between the two regex libraries.

I'm not sure how big the problem is. On one hand lots of languages on github work without a problem. On the other, they also got a ticket for "Outstanding Grammar Issues".

In principle, we can extract all individual regexes from the syntax file, compile them both for PCRE and oniguruma and run against snapshot test cases. Maybe it is a bit of an overkill, but still better than checking the results by hand.

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