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

Add X++ support #2339

Merged
merged 17 commits into from Feb 14, 2023
Merged

Add X++ support #2339

merged 17 commits into from Feb 14, 2023

Conversation

andrewschmidt-a
Copy link
Contributor

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

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:

Copy link
Collaborator

@Anteru Anteru left a comment

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 :)

pygments/lexers/dotnet.py Outdated Show resolved Hide resolved
@andrewschmidt-a
Copy link
Contributor Author

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

@andrewschmidt-a
Copy link
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
Copy link
Contributor

jeanas commented Feb 10, 2023 via email

@jeanas
Copy link
Contributor

jeanas commented Feb 13, 2023

This fails CI, apparently a missing r string prefix.

@andrewschmidt-a
Copy link
Contributor Author

andrewschmidt-a commented Feb 13, 2023

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

pygments/lexers/dotnet.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jeanas jeanas left a comment

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
Contributor Author

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

pygments/lexers/dotnet.py Outdated Show resolved Hide resolved
@andrewschmidt-a
Copy link
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
Copy link
Contributor

jeanas commented Feb 13, 2023

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

@jeanas
Copy link
Contributor

jeanas commented Feb 13, 2023

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
Copy link
Member

birkenfeld commented Feb 14, 2023

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
Copy link
Contributor

jeanas commented Feb 14, 2023

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
Member

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

@jeanas
Copy link
Contributor

jeanas commented Feb 14, 2023

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.

None yet

4 participants