Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Conversation

mkhl
Copy link
Contributor

@mkhl mkhl commented Mar 8, 2016

This fixes for…in loop handling by

  • updating the grammar to match them more accurately
  • adding a (very simple) snippet to insert them

This pull request includes #35, which it would otherwise conflict with.

mkhl added 3 commits March 8, 2016 13:15
The regexp engine and the shell grammar don't agree on what comprises
a “word boundary”. This leads to problems where a command or path
ending in a shell keyword would be interpreted as that keyword.

This change replaces `\b` word boundary matchers with lookahead/-behind
matchers on whitespace, line breaks, and command separators (";" and "&").

It also eliminates ad-hoc custom word boundaries, where subsets of [-=/]
were pre- or appended to some word boundaries,
which fixed some similar problems.
The `in` keyword of the “for…in” loop is optional.
In contrast, the variable name preceding it is required
and must be a valid variable name, not an arbitrary string.
The for…in loop is much more common than the C-style for loop.
The snippets package doesn’t seem to support transformations in
snippet output yet, so this change adds a bare bones version
that always inserts the (optional) in keyword.
@winstliu
Copy link
Contributor

winstliu commented Mar 8, 2016

@mkhl please keep the PRs separate in order to make reviewing easier. Thanks!

@mkhl
Copy link
Contributor Author

mkhl commented Mar 8, 2016

@50Wliu I’m not quite sure how to keep them separate, since they are not independent.

Would it be OK if I closed this one and reopened it once we’re done with #35?

@winstliu
Copy link
Contributor

winstliu commented Mar 8, 2016

@mkhl What I would probably do is include the fixes to the for-in loop in this PR using existing \\b regexes, then fix those regexes later in #35. Alternatively, going the other way would be fine as well. Your call :).

@mkhl
Copy link
Contributor Author

mkhl commented Mar 8, 2016

@50Wliu My intention was to go the other way because I believe #35 to be the more obvious (if larger) change. Your comments indicate that I haven’t done a good job though. :-)

I think holding off on this until after #35 is done would cause the least effort. Whether you’d prefer I close this for that period is your call.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants