-
Notifications
You must be signed in to change notification settings - Fork 522
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: Make goose annotations case-insensitive #704
Conversation
@mfridman please take a look, when you have a chance |
@mfridman, Could you please run the pipeline? I fixed linter warning |
Thanks for putting this together, I'll try my best to get a review within a week. |
internal/sqlparser/parser.go
Outdated
// Allowed annotations: Up, Down, StatementBegin, StatementEnd, NO TRANSACTION, ENVSUB ON, ENVSUB OFF | ||
func extractAnnotation(line string) (annotation, error) { | ||
// Remove leading and trailing whitespace from line. | ||
cmd := strings.TrimSpace(line) |
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.
I've been going back and forth on whether we should do this.
I.e., a goose annotation must begin with -- +goose
, with no leading white space.
If we do, we'll need to update the blog (I want to move some of this "blog" content into formal docs)
https://pressly.github.io/goose/blog/2022/overview-sql-file/#quick-start
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.
I can remove this line and add a check if the line has a leading space and throw an appropriate 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.
That's probably the right thing:
- Only allow
-- +goose <annotation>
without leading space - Raise an error if we detect a
-- +goose
annotation with leading whitespace like\s+-- \+goose
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.
I'll add changes to cover this logic
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.
@mfridman I refactored PR to allow annotations only without leading whitespace
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. 🎉
@mfridman Thank you for your review! |
This PR does two things:
Closes #678