Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pjdufour-truss
Copy link
Contributor

The DisableExec can be used to disable execution of exec("....") 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.

@pjdufour-truss pjdufour-truss requested a review from a team as a code owner June 14, 2019 20:06
Copy link

@ghost ghost left a 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 {
Copy link

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

Suggested change
func NewBubblerWithDisabledExec(t Translator) *Bubbler {
// NewBubblerWithDisabledExec ...
func NewBubblerWithDisabledExec(t Translator) *Bubbler {

@pjdufour-truss
Copy link
Contributor Author

It would be great to get this merged! Please let me know if I can answer any questions.

@stanislas-m stanislas-m self-requested a review June 18, 2019 21:58
Copy link
Member

@stanislas-m stanislas-m left a 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. :)

@pjdufour-truss
Copy link
Contributor Author

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?

@pjdufour-truss
Copy link
Contributor Author

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.

@stanislas-m stanislas-m self-assigned this Jul 13, 2019
@stanislas-m stanislas-m added the s: hold shouldn't be closed, but should wait for further work. label Jul 13, 2019
@sio4 sio4 added the s: triage label Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: hold shouldn't be closed, but should wait for further work. s: triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants