-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
C# improvements #1444
C# improvements #1444
Conversation
Generic methods now allow for nested generic type parameters. A generic constructor's type name is no longer highlighted as a function.
Added support for type names with generic parameters. Type parameters are now also marked as class names.
Minor note: you'll need to commit the minified file for C#. Otherwise, I'll defer to my co-maintainers to review the Regex here. |
Adds support for binary integers, integer prefixes and the `_` digit separator
I added support for |
`true` and `false` are now `boolean` Support for `as` and `is`
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.
Hi! Thanks for contributing.
During this review, I struggled a bit to understand what each pattern was trying to do. It would be great if the test files reflected the structure of the language definition. That is, there should be one test file per pattern name ("number_feature.test"
, "keyword_feature.test"
), and if you want you can split a complicated pattern (I'm looking at you, "class-name"
) between multiple test files but please refer each one to its corresponding pattern by "naming" them using a comment.
Doing so, the tests files might get a bit redundant sometimes, when a pattern needs to be next to another pattern to be valid. But at least it makes it obvious what your regexps are trying to match.
Maybe I should explain the main part that matches almost every class name. That is this: /[A-Z]\w*(?:\.\w+)*(?:<(?:[^>\n=;{}]+|>(?=\s*(?:\[\s*(?:,\s*)*\]\s*)*[,.>?)]))+?>)?(?:\.\w+)*(?:\[\s*(?:,\s*)*\])*/ Seems complicated? Things get a little easier when you take out the part responsable for generics. Then we get: /[A-Z]\w*(?:\.\w+)*(?:\[\s*(?:,\s*)*\])*/ Now that's friendlier. (I removed the redundant Ok back to the part for generics: /(?:<(?:[^>\n=;{}]+|>(?=\s*(?:\[\s*(?:,\s*)*\]\s*)*[,.>?)]))+?>)?/ The main problem here is that we try to accomplish the impossible. We try to match structures like And fun I had. What I did, was to say that there are only a limited number of characters which can follow after a I know that this is far from perfect but it work quite reliably in all of my tests and code examples. |
I'm also thinking of adding real support for nullables, because with C# 8.0 coming it might be important. |
Simplified type expressions. This dropps support for tupels. Moved return type to fix a bug with shadowing methods. Added and updated tests.
Do people really do 5 nested levels of generics in real-world code? The regexp could certainly be faster if we limited Prism's support to a certain number of nested levels (like 2... or maybe 3. Above, the regexp will get really ugly). |
I certainly hope not, but generics can get messy. (Hopefully not that messy...)
/<(?:[^<>]+|<(?:[^<>]+|<(?:[^<>]+|<[^<>]+>)+>)+>)+>/ I settled for 4 as the maximum depth. A depth of 3 should be enough for everything I do and 4 should satisfy even hardliners. It's probably a little overkill but better safe than sorry. Btw. That beauty above is 9 characters less than its predecessor. |
We're definitely not! The regexp in your last message looks ok to me. I'll review again once the PR gets updated. |
Now there is a small problem. Consider the regex for generic methods simplified for a max depth of 2: /\w+\s*<(?:[^<>]+|<[^<>]+>)+>(?=\s*\()/ Just try to match the text
and see how long it takes for you. It will terminate returning that no match was found, but only after exponential time. |
Any updates on this PR? Would be awesome if it could get merged within a few months. I realize you seem to be swamped with work. |
So here's an update for this PR. Aside from explicit implementation (which I can't do without #1679) Added features include:
|
any updates on this? |
It just needs someone to review. |
Thanks for the review @Golmote! Apparently, Travis is taking the day off. I'll try to get it to run and then merge. |
Ok. Travis actually ran and succeeded but the status isn't displayed here on GH. |
A complete rewrite of the C# language to support almost every feature of C# 8.0.
This PR is mainly about an improved detection of type names and better support for generic parameters in C#.
It also adds support for the
nameof
operator and now highlights type parameters the same as classes.