Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
enh(r) Add operators and punctuation #3195
enh(r) Add operators and punctuation #3195
Changes from 7 commits
f6e5234
9524023
4c746d9
684114d
f508d48
252483c
c4d7b81
bf1dc80
cb7a392
f165d24
ea9e3ff
3dc014c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Please add the full assignment regex back or is there some reason this had to change? More explicit is better than less typically.
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.
The previous assignment regex was only used as a relevance boost, and only matches a small subset of all possible assignments, namely a subset of all legal identifiers, plus whitespace, plus assign, plus whitespace.
We can’t use this for highlighting since it misses many cases:
x[1] <- 2
)I’m actually not sure why this regex was so overly specific even for relevance boosting: was it to avoid clashing with other languages? At any rate, assignment expressions in R can be (and routinely are, in actual code) quite complex.
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.
Yes. The boost was intended to be narrow so as not to trigger false positives. If it only catches some common cases that's sufficient. It's not trying to be exhaustive. (relevance boosts often don't need to be to help)
If you wanted to use a proper IDENT vs SIMPLE_IDENT that would be an improvement.
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.
Sorry, commit bf1dc80 included an unnecessary, redundant declaration, I’ve removed that in cb7a392. The result should be alright:
<-
is now handled in all situations, but the signal boost is only applied with the previous situation (except using the fullIDENT_RE
).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.
Long-term I do worry this makes the tests much harder to read, but I don't have a quick solution.
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.
Yeah, it hasn’t been great during debugging. :-(
Maybe I should just replace the commas by spaces?
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.
Maybe. That wouldn't help in actual code examples though... :-)