-
Notifications
You must be signed in to change notification settings - Fork 804
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
feat(compiler): Parse query parameter metadata from comments #2850
Conversation
lines = append(lines, t) | ||
} | ||
return strings.Join(lines, "\n"), comments, s.Err() | ||
} | ||
|
||
func CleanedComments(rawSQL string, cs CommentSyntax) ([]string, error) { |
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.
We've now implemented comment parsing/cleaning in three places. I was hoping to consolidate but walked it back in favor of a separate code path.
It's some unfortunate technical debt though, since at some point someone is going to complain that we don't support multi-line slash-star comment syntax and we'll have to refactor all of this...
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.
So a comment like
/*
name: Foo :one
@param bar? text
*/
won't work? If that's the case, we'll want to have a fast follow to enable that.
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.
Looking over the implementation of CleanedComments, I don't think it's be too hard to add support for multiline comments. But let's address that in a future PR.
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 the multi-line comment syntax has never worked but there wasn't really a reason for anyone to ask before this change.
Agree we can implement later
@@ -15,6 +15,12 @@ type Edit struct { | |||
OldFunc func(string) int | |||
} | |||
|
|||
type CommentSyntax struct { |
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.
Handling comments seems like a source code thing, so I popped this in the source
package. The new CleanedComments
func I wrote depends on comment syntax, and we already have a func StripComments
(which doesn't depend on comment syntax but I think that's just an oversight/bug).
lines = append(lines, t) | ||
} | ||
return strings.Join(lines, "\n"), comments, s.Err() | ||
} | ||
|
||
func CleanedComments(rawSQL string, cs CommentSyntax) ([]string, error) { |
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.
So a comment like
/*
name: Foo :one
@param bar? text
*/
won't work? If that's the case, we'll want to have a fast follow to enable that.
lines = append(lines, t) | ||
} | ||
return strings.Join(lines, "\n"), comments, s.Err() | ||
} | ||
|
||
func CleanedComments(rawSQL string, cs CommentSyntax) ([]string, error) { |
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.
Looking over the implementation of CleanedComments, I don't think it's be too hard to add support for multiline comments. But let's address that in a future PR.
Groundwork for #2800 (explicit type annotations in query comments metadata).
There are lots of tests that failed while I was refactoring this, so I have a fairly high degree of confidence that this refactor doesn't break anything. The changed endtoend test output is due to parsing bugs I fixed.
I chose to add a new
Metadata
struct type to house the growing set of stuff we associate with a query, but I don't have to so let me know if an alternative approach is preferred.