Treat bare dollar sign as literal and dont interpolate #16
Conversation
parser.go
Outdated
| if strings.HasPrefix(p.input[p.pos:], `$ `) { | ||
| p.pos += 2 | ||
| expr = append(expr, ExpressionItem{Text: `$ `}) | ||
| continue | ||
| } |
There was a problem hiding this comment.
we might wanna check for common non-space whitespace too, the \ns and \rs of the world
parser.go
Outdated
| // Ignore empty variable references | ||
| if strings.HasPrefix(p.input[p.pos:], `$ `) { | ||
| p.pos += 2 | ||
| expr = append(expr, ExpressionItem{Text: `$ `}) | ||
| continue | ||
| } |
There was a problem hiding this comment.
This is a bandaid fix for a specific case - arguably we should be treating a $ as a literal if there is no next character, or the next character is anything we don't parse as an expansion later on. Something like:
// If we run into a dollar sign, what we do with it depends on what comes next
if c == '$' {
if p.pos == len(p.input)-1 {
// It can't be an expansion.
p.pos++
expr = append(expr, ExpressionItem{Text: "$"})
continue
}
if c2 := p.peekRune(); c2 == '{' || unicode.IsLetter(c2) {
// It's ${ or $something ... probably an expansion!
expansion, err := p.parseExpansion()
if err != nil {
return nil, err
}
expr = append(expr, ExpressionItem{Expansion: expansion})
continue
}
}Note, though, that this only doesn't work because peekRune can't peek the rune after the current position...
jordandcarter
force-pushed
the
ps-215-treat-bare-dollar-sign-as-literal
branch
from
8cca45e to
4c53760
Compare
|
What about this approach? |
jordandcarter
force-pushed
the
ps-215-treat-bare-dollar-sign-as-literal
branch
from
4c53760 to
28db7ac
Compare
parser.go
Outdated
| } | ||
|
|
||
| // if not a letter, it's a literal dollar sign | ||
| if !unicode.IsLetter(c) && !unicode.IsNumber(c) { |
There was a problem hiding this comment.
I think strings starting with numbers aren't considered to be identifiers (scanIdentifier only checks unicode.IsLetter)
There was a problem hiding this comment.
Yeah I only added IsNumber to avoid this test. Should we raise an error for a number, or just treat as literal? My naive opinion is to treat as literal and not raise.
There was a problem hiding this comment.
I agree, for our purposes (pipeline interpolation) it's a literal - similar to $(...).
parser.go
Outdated
| err = fmt.Errorf("Expected expansion to start with $, got %c", c) | ||
| return |
There was a problem hiding this comment.
This function is just about too long for naked returns, IMO.
| err = fmt.Errorf("Expected expansion to start with $, got %c", c) | |
| return | |
| return nil, false, fmt.Errorf("Expected expansion to start with $, got %c", c) |
|
While you were reviewing just now @DrJosh9000 I tried another approach which just returns |
|
That one looks good! I think my comment about identifiers not starting with numbers still applies, but otherwise ✅ |
…literal dollar sign
…tead of raising an error
$as literal and don't raise an error trying and failing to interpolate it$<IDENTIFIER>) not starting with a letter or brace as literal and don't raise error.