-
Notifications
You must be signed in to change notification settings - Fork 57
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
Update copied changes from sqlds #862
Conversation
161bbd3
to
ce99188
Compare
ce99188
to
a811d12
Compare
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.
LGTM 👍
if err != nil { | ||
return rawSQL, err | ||
} | ||
|
||
rawSQL = strings.ReplaceAll(rawSQL, match[0], res) | ||
rawSQL = strings.ReplaceAll(rawSQL, match.Name, res) |
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.
random note, this is not part of the diff.. here we use ReplaceAll()
.
in sqlds
, it's Replace()
:
https://github.com/grafana/sqlds/blob/dda2dc0a54b128961fc9f7885baabf555f3ddfdc/macros.go#L258
what do you think, which one is "more correct"?
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.
Ah, I actually made the change intentionally in my original PR; this time around I forgot, but then the linter in this repo told me to change it! Looks like this comes from gocritic:
❯ mage lint
data/sqlutil/macros.go:215:13: wrapperFunc: use strings.ReplaceAll method in `strings.Replace(rawSQL, match.full, res, -1)` (gocritic)
rawSQL = strings.Replace(rawSQL, match.full, res, -1)
^
Error: running "golangci-lint run ./..." failed with exit code 1
I also prefer ReplaceAll
as I think it makes the intention more clear, but it's a pretty minor thing.
What this PR does / why we need it:
Functionality from
sqlds/macros.go
andsqlds/query.go
was brought into this repo'ssqlutil
as part of https://github.com/grafana/oss-plugin-partnerships/issues/94. Since then some features and bugfixes have been added to the originals. These changes need to be brought in to sqlutil as well to bring the functionality here up to date, so sqlds can be migrated to use this library instead of its own versions (grafana/sqlds#85).For this PR I've tried as much as possible to copy the
sqlds
code over unchanged. #863 applies some refactoring and fixes a couple of bugs.Which issue(s) this PR fixes:
Addresses #858.