Distinguish between /regexp/ and "regexp" in stringified output #222
Conversation
internal/ast/ast.go
Outdated
| r := &RegExpr{e.Value} | ||
| return r.String() |
There was a problem hiding this comment.
Nit suggestion (untested, but I think it should work):
| r := &RegExpr{e.Value} | |
| return r.String() | |
| return RegExpr.String(e.Value) |
There was a problem hiding this comment.
It won't, because (*RegExpr).String takes a *RegExpr as an argument, not a string.
There was a problem hiding this comment.
Building a RegExpr object here still feels a little weird -- can we factor out the content of RegExpr.String into formatRegex and call it from both places?
internal/ast/ast.go
Outdated
| type StrExpr struct { | ||
| Value string | ||
| Value string | ||
| Regexp bool |
There was a problem hiding this comment.
Naming nit: can we call this IsRegexp for clarity?
There was a problem hiding this comment.
We could, but that's so not-Go
There was a problem hiding this comment.
Maybe we should document it better?
There was a problem hiding this comment.
Looks like your mostly right that it's "not Go", though not entirely, for example, the Is* fields here. However, we have another bool in this package, IncrExpr.Pre, that doesn't have the Is, so let's go with that.
However, on closer look, can we please call it Regex bool? The "regexp" with a 'p' kind of refers to Go's "regexp" package, whereas we I tend to call it "regex" in an AWK context (for example, the RegExpr.Regex field itself).
benhoyt
changed the title
|
@benhoyt Could you create a new tag so that Keep in mind that this change breaks code that has an expression like |
|
That's okay: 1) the |
|
New version released: https://github.com/benhoyt/goawk/releases/tag/v1.26.0 |
Fixes issue #221