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

Update Fennel keywords to catch up to version 0.6.0. #1535

Merged
merged 5 commits into from Sep 7, 2020

Conversation

technomancy
Copy link
Contributor

The keywords in the current Fennel lexer are a bit behind; this catches it up to the latest release.

Also removes some disallowed identifier characters we accidentally left in previously.

Copy link
Member

@birkenfeld birkenfeld left a comment

Choose a reason for hiding this comment

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

JFYI, the u prefix can be omitted now that we're py3-only.

@birkenfeld
Copy link
Member

However, the example file doesn't seem to like your changes? Can you verify that https://github.com/pygments/pygments/blob/master/tests/examplefiles/fennelview.fnl is still valid?

Remove support for single-quoted strings.

Update fennelview example to latest version of library.
@technomancy
Copy link
Contributor Author

Thanks! Sorry for the noise; I think this should take care of it now. I had forgotten the previous patch was from such an old version, so the example needed to be brought up to date.

Copy link
Member

@birkenfeld birkenfeld left a comment

Choose a reason for hiding this comment

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

The changes themselves look good now, we just should resolve the string regex issue.

@@ -2670,7 +2675,6 @@ class FennelLexer(RegexLexer):
(r'-?\d+', Number.Integer),

(r'"(\\\\|\\"|[^"])*"', String),
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I just saw this right now: this regex is prone to catastrophic backtracking, we should fix it while at it. It should look like

"(\\\\|\\"|[^"\\])*"

with alterations as needed for other allowed escape sequences. If they are complex, a separate state can be used as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's literally the third time in a week or so we got the same regex of doom :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, well really that regex was added years ago; copied from an existing lexer IIRC. But I'm glad to see it gone.

@birkenfeld
Copy link
Member

I made another small fix to account for other allowed escapes.

@birkenfeld birkenfeld merged commit 2b3ab6d into pygments:master Sep 7, 2020
@birkenfeld birkenfeld added the changelog-update Items which need to get mentioned in the changelog label Sep 7, 2020
@technomancy
Copy link
Contributor Author

Thanks! As you can probably tell, I've never written Python before so I appreciate the cleanup.

@technomancy technomancy deleted the fennel-new-keywords branch September 7, 2020 16:06
@birkenfeld
Copy link
Member

Thanks even more for the contribution then!

@Anteru Anteru removed the changelog-update Items which need to get mentioned in the changelog label Sep 8, 2020
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.

None yet

3 participants