Skip to content

Add X++ support#2339

Merged
jeanas merged 17 commits into
pygments:masterfrom
andrewschmidt-a:add-xpp-support
Feb 14, 2023
Merged

Add X++ support#2339
jeanas merged 17 commits into
pygments:masterfrom
andrewschmidt-a:add-xpp-support

Conversation

@andrewschmidt-a

@andrewschmidt-a andrewschmidt-a commented Feb 8, 2023

Copy link
Copy Markdown
Contributor

X++ is a language developed and maintained by Microsoft. It compiles down to CIL. It is considered a .NET language. This is used exclusively within Microsoft's ERP product called Dynamics 365 and its partner ecosystem.

Reference links:

@Anteru Anteru left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the contribution. Can you please add an example file as well and generate "golden" test output for it? Reviewing regexes without seeing them run is hard :)

Comment thread pygments/lexers/dotnet.py Outdated
@andrewschmidt-a

Copy link
Copy Markdown
Contributor Author

@Anteru, thanks for the feedback I have included a test and its output

@andrewschmidt-a

Copy link
Copy Markdown
Contributor Author

@Anteru , I tried to run mapping for my change because the build failed with this error
Please run "make mapfiles" and add the changes to a commit.

Not sure how it was different than my manual change but I committed it.

@jeanas

jeanas commented Feb 10, 2023 via email

Copy link
Copy Markdown
Contributor

@jeanas

jeanas commented Feb 13, 2023

Copy link
Copy Markdown
Contributor

This fails CI, apparently a missing r string prefix.

@andrewschmidt-a

andrewschmidt-a commented Feb 13, 2023

Copy link
Copy Markdown
Contributor Author

I found the CI pipeline and ran the commands myself, I think this should be covered. RegexLint didnt give me an affirmative output but I think it passed

image

Comment thread pygments/lexers/dotnet.py Outdated

@jeanas jeanas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall this looks quite well thought out to me, with two reservations:

  • I'm not sure the Unicode level configurability is worth the code, see comment inline.

  • While testing on large files (basically with for i in {1..1000}; do echo tests/examplefiles/xpp/test.xpp >> bigtest.xpp; done), I experienced RecursionErrors. This is because the lexer sometimes recurses with using(this) for way too much code. For example, in r'(\s*)\b(else|if)\b(.*)', the .* part will match everything after the if in the file (since you have DOTALL). Please fix this and check that it works on a large file.

@andrewschmidt-a

Copy link
Copy Markdown
Contributor Author

@jeanas , thanks for the suggestions. I have tested with a large file and am not experiencing a recursion error.

Comment thread pygments/lexers/dotnet.py Outdated
@andrewschmidt-a

Copy link
Copy Markdown
Contributor Author

Any suggestions to resolve the REGEXLint issue? I use the full set and it complains about null characters not being supported. This is the same regex used in the CSharpLexer, but I think it does not complain about it cause the basic level is default. If I set mine to the basic level, the regex errors go away.

@jeanas

jeanas commented Feb 13, 2023

Copy link
Copy Markdown
Contributor

I've submitted a fix to regexlint for this. See thatch/regexlint#49

@jeanas

jeanas commented Feb 13, 2023

Copy link
Copy Markdown
Contributor

Ah, I just noticed that CI uses the fork https://github.com/pygments/regexlint, not the original https://github.com/thatch/regexlint. @birkenfeld What is the history behind this?

@birkenfeld

birkenfeld commented Feb 14, 2023

Copy link
Copy Markdown
Member

Probably just switched to a repo I had access to while I was working on the updates for thatch/regexlint#40 - after that was merged I should have switched back to upstream.

@jeanas

jeanas commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

In the meantime, I've added a workaround so we can merge this. It only affects code with null characters, which is quite unimportant to get right.

@jeanas
jeanas merged commit 0ae4bc8 into pygments:master Feb 14, 2023
@birkenfeld

Copy link
Copy Markdown
Member

NUL chars can be escaped on the regex level with r"\x00" anyway, no?

@jeanas

jeanas commented Feb 14, 2023

Copy link
Copy Markdown
Contributor

Ah, right, I forgot about this. Well, eventually, we'll use regex anyway, which supports matching on Unicode character classes 😉

@Anteru Anteru added this to the 2.15.0 milestone Mar 25, 2023
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.

4 participants