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

before/after new hooks #109

Closed
wants to merge 6 commits into from
Closed

Conversation

chb0github
Copy link
Contributor

issue #106

@harawata
Copy link
Member

Thank you @chb0github !

The existing HookContext is designed for database operation hooks (up, down, etc.) and is not appropriate for local operation hooks (new, script, etc.), so please do not add it to hookBindings.
I think that new-hooks need just two strings 'description' and 'filename'.
We should either put them into hookBindings directly or create a new context object (e.g. NewHookContext).

@chb0github
Copy link
Contributor Author

There is only one element put into the bindings map and that is the HookContext - in my opinion the bindings map should have always been the hook context without a different object.

As for what should be supplied in the context overall (and it's appropriateness) to accessing the DB: Why artificially limit before_new or after_new from having access to the DB? You could actually use the before_new to basically entirely intercept and modify the Change that will be applied. It might need to talk to the DB to get a meta record - better to not preclude such a positive use case without clear and compelling reason why.

On the new_hook in general I would offer that it

  1. Receives the change to potentially be applied
  2. Returns a new change (or the existing one) which is then applied. This could mean changing the file location, the name, the description, etc.

So, let's say you want the description or to contain a JIRA ticket number (we do) and the user making the change by default (we do), I could do both and the result is THEN turned into the file to be applied. in our case the JIRA ticket is determined from the local branch.

If you would like a NewHookContext then what do you image should be in it?

  1. The proposed Change
  2. A connection to the DB (my vote)

@harawata
Copy link
Member

harawata commented Dec 24, 2017

It might need to talk to the DB to get a meta record

It's a bit too hypothetical, but when executing migrate new, the DB you can access via HookContext will be the one in the development environment and it may not be the one that contain such data in normal uses.
It is totally possible to access any DB without HookContext and I think it's good enough for now.

So, let's say you want the description or to contain a JIRA ticket number (we do) and the user making the change by default (we do), I could do both and the result is THEN turned into the file to be applied.

As SelectedPaths and filename are provided, it should be possible to modify the file content and move it in after_new hook.
Modifying Change is not possible in the existing hooks either, by the way.

@@ -50,7 +51,11 @@ public Connection getConnection() throws SQLException {
* Source of the SQL to execute.
*/
public void executeSql(Reader reader) {
scriptRunner.runScript(reader);
try {
scriptRunner.runScript(reader, connectionProvider.getConnection());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to remove this - that change isn't merged into mybatis yet

} finally {
runner.closeConnection();
connection.close();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove pending mybatis changes

@chb0github
Copy link
Contributor Author

chb0github commented Dec 30, 2017

About the most recent changes:

  1. Created a new Hook type. Since the concept of before_each and after_each doesn't really apply in this situation I restructured hooks. It's all backward compatible. There is now BasicHook that has before and after
  2. I isolated the code for before/after. Basically, it figures out what it the proposed change file should be, offers it to the before hook along with the running environment (thus addressing @harawata comments) and then creates the file based on the template arrangement (as before) but crucially, the hook can change the location of the file in the change. Demonstrated in before_new.js
  3. I tried to follow the current code thinking/methods. So, I added static "helper" methods in BaseCommand in a few cases.
  4. Following the Null Object Pattern I implemented a couple of no-op classes so I could eliminate redundant null checked. This makes the code permanently null proof.

Working on the tests.

@chb0github
Copy link
Contributor Author

Does anyone have any input?

@hazendaz
Copy link
Member

hazendaz commented Jan 1, 2018

@chb0github Hang tight! I reached out to the broader group in the dev mailing list to get input. I usually only deal with build related things so most of what you are doing I've never actually worked with so I unfortunately don't have much input one way or the other but appreciate the activity and improvements.

@chb0github
Copy link
Contributor Author

hurray! new hooks builds!

@chb0github chb0github mentioned this pull request Jan 3, 2018
@chb0github
Copy link
Contributor Author

Closing out in favor of PR #113

@chb0github chb0github closed this Jan 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants