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

Improve parameter type matching regex #680

Merged

Conversation

seraku24
Copy link
Contributor

Fixes (again) #657 as well as issues brought up in #670.

  • Corrects the order of type, template arguments, nullable type specification and array specifications.
  • Ensures array specification support all forms (simple arrays, multi-dimensional arrays, arrays of arrays).

Fixes (again) dotnet#657 as well as issues brought up in dotnet#670.

- Corrects the order of type, template arguments, nullable type specification and array specifications.
- Ensures array specification support all forms (simple arrays, multi-dimensional arrays, arrays of arrays).
@seraku24
Copy link
Contributor Author

And for comparison, here are the results of the regex on a pathological test case:

Current behavior
omnisharp-syntaxhighlighting-currentregex

Updated behavior
omnisharp-syntaxhighlighting-updatedregex

@ivanz
Copy link
Contributor

ivanz commented Aug 18, 2016

@seraku24 You may want to update the regex to handle unsafe pointers (byte* ,etc) too since you are at it. Relates to those two issues: #101 and microsoft/vscode#1093

@ivanz
Copy link
Contributor

ivanz commented Aug 18, 2016

@seraku24 Also one more thing is to handle @ too e.g. string @class

@ivanz
Copy link
Contributor

ivanz commented Aug 18, 2016

@seraku24 Final thing - might be worth checking if your regex also fixes the method return value highlighting problem here: #33

- Unsafe pointer types (e.g. `char**`).
- Escaping reserved keywords for variable names (e.g. `int @class`)
@seraku24
Copy link
Contributor Author

Okay, @ivanz. I have adjusted the regex to support unsafe pointers as well as reserved keyword escaping:

omnisharp-syntaxhighlighting-updatedregexpart2

Regarding return types, it seems that some types are highlighting correctly already but there are others that are not:

omnisharp-syntaxhighlighting-returntypes

That said, we might want to keep the scope of this PR to just addressing the issues with highlighting parameters, which I believe we have done.

@ivanz
Copy link
Contributor

ivanz commented Aug 18, 2016

Looks good to me, but @DustinCampbell is the one with the review and merge powers and I believe he is based in the US, so it's just about morning there. Congrats on your first GitHub pull request 🎉

@seraku24
Copy link
Contributor Author

Well, compared to other source/version control systems, git and GitHub have probably been the easiest to work with, especially when compared to Product Studio or TFS. (:

Thanks for the help and encouragement. I am just glad to be able to offer some of my regex experience at the very least.

@DustinCampbell
Copy link
Member

LGTM

@DustinCampbell DustinCampbell merged commit dc8cd65 into dotnet:master Aug 18, 2016
@DustinCampbell DustinCampbell added this to the 1.4 milestone Aug 18, 2016
@seraku24 seraku24 deleted the fix-parameter-syntax-highlighting branch August 19, 2016 05:16
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