-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
DisableExec flag #54
base: main
Are you sure you want to change the base?
DisableExec flag #54
Conversation
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.
Found some fixes!
P.S. share your ideas, feedbacks or issues with us at https://github.com/fixmie/feedback (this message will be removed after the beta stage).
} | ||
} | ||
|
||
func NewBubblerWithDisabledExec(t Translator) *Bubbler { |
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.
exported identifier "NewBubblerWithDisabledExec" should have comment
func NewBubblerWithDisabledExec(t Translator) *Bubbler { | |
// NewBubblerWithDisabledExec ... | |
func NewBubblerWithDisabledExec(t Translator) *Bubbler { |
It would be great to get this merged! Please let me know if I can answer any questions. |
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.
To be honest, I think there is a real issue in the way the exec
function is processed. It's not because of you, but thanks to your contribution I can see the problem better now: while most of the functions are just translating fizz statements to their equivalent in SQL (so the SQL query is not executed right now), the exec
command is processed while parsing the fizz contents.
That behavior is not correct, since you can expect to execute a SQL command, then execute an external command depending on this SQL; but the exec
statement is processed before anything else.
We should rework and fix that so everything is processed in the correct order, so I'm afraid you're trying to patch the wrong thing. :)
Thank you very much for the response and explanation. I'm new to pop/fizz and don't fully understand its architecture, so I think I'd have to leave it to an expert to make that significant improvement. Could we create an issue where this can be tracked until resolution? |
So what's the way forward for this request? Merge this PR and add TODO? Or do you see a fix in the near term? Linting fizz files is a key developer experience improvement. Thanks for your thoughts. |
The
DisableExec
can be used to disable execution ofexec("....")
within fizz files, such as during syntax validation. This is simple approach and preferable to a regex replace approach.Default behavior is preserved and the only new public function is
NewBubblerWithDisabledExec
.