Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(sql): add SQL bootstrapper #4376
feat(sql): add SQL bootstrapper #4376
Changes from 4 commits
fc3cbdb
7543a42
f5628a6
a7b83aa
0e99496
cfd07a3
6cf3ab8
d8cb9e6
b154558
5ff7285
fd84edf
c717415
ff62166
ee45e97
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this method should be put in the interface (that will save the explicit cast in the
SqlSchemaBootstrapperExtension
)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 don't want this method in the interface, for it could entice contributing modules to invoke it, which could potentially cause problems. The cast is ugly, but it should be possible to simply use the variable (as opposed to the
getBootstrapper()
method) in theprepare()
phase.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.
ok, I understand it, but this necessity is not explicit (and being misinterpreted, e.g. from a future-me :) ), that could be achieved by keeping only the
addStatement
method in the bootstrapper and moving the execution logic to a module-private service, that won't be registered on the runtime, this way theexecuteSql
will effectively become not callable from other modules.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.
thats a lot of ceremony for one single method. if
executeSql()
is package-private and documented, it should be clear enough what is done.In addition, I didn't put the interface in an SPI package on purpose, because it is not intended as an extension point anyway.
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 tried it, and it looks OK, but I'm not really sure splitting such a trivial task in two classes (and two test classes) is really worth it TBH. LMK what you think.