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

Support optional props in TypeScript lexer #1393

Merged
merged 2 commits into from Jan 23, 2020

Conversation

pyrmont
Copy link
Contributor

@pyrmont pyrmont commented Jan 15, 2020

TypeScript allows a user to mark a property as an optional when defining an interface by appending a question mark to the property name. Rouge's TypeScript lexer doesn't currently support this because the lexer merely uses the states defined in the JavaScript lexer.

This PR prepends a rule to the statement state that will match a property name ending with a ?. This will allow matches outside of the interface definition but as syntax checking is a non-goal of Rouge this is not considered to be a problem.

The TypeScript visual sample is updated to include an example.

This fixes #1391.

@pyrmont pyrmont self-assigned this Jan 15, 2020
@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Jan 15, 2020
@pyrmont
Copy link
Contributor Author

pyrmont commented Jan 15, 2020

@aaronadamsTO I think this fixes the problem you identified in #1391.

One thing I did note was that there seems to be a difference in how the question mark should be lexed. GitHub's syntax highlighter highlights the question mark differently to the prop name whereas the highlighting in the official TS docs doesn't.

Do you have an opinion on this? I'm currently following the TS docs but perhaps it would make more sense to be consistent with GitHub.

@aaronadamsCA
Copy link

aaronadamsCA commented Jan 21, 2020

@pyrmont Looking around at other lexers, there seems to be consistency in treating ? and : the same, as keywords, separate from the property name.

That said, I don't really have a strong opinion here! Either outcome seems fine to me.

@pyrmont
Copy link
Contributor Author

pyrmont commented Jan 22, 2020

@aaronadamsTO It seems a little strange to me to tokenise ? as a Keyword. I was going to go with Punctuation. Would that be wrong, do you think?

@pyrmont pyrmont added discussion-open and removed needs-review The PR needs to be reviewed labels Jan 22, 2020
@aaronadamsCA
Copy link

@pyrmont Makes sense to me. Sorry, I meant "keyword" in the TextMate grammar sense - I'm new to Jekyll!

@pyrmont
Copy link
Contributor Author

pyrmont commented Jan 22, 2020

@aaronadamsTO Got it! Will update the token and merge :)

@pyrmont pyrmont merged commit 09c2b8b into rouge-ruby:master Jan 23, 2020
@pyrmont
Copy link
Contributor Author

pyrmont commented Jan 23, 2020

@aaronadamsTO OK—got it merged! The next release of Rouge is scheduled to be v3.16.0. That will come out 11 February :) Thanks for the help!

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.

TypeScript interface optional properties are not recognized
2 participants