-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add more hooks #27
base: main
Are you sure you want to change the base?
Add more hooks #27
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.
Hi, this is very nice addition, however i have a few comments/questions about the number of hooks. What do you think?
for proj_id in &self.proj_writes { | ||
if let Some((root, hooks)) = args.hooks.get(proj_id) { | ||
hooks.execute_pre_begin(root)?; | ||
} | ||
} | ||
|
||
for proj_id in &self.proj_writes { | ||
if let Some((root, hooks)) = args.hooks.get(proj_id) { | ||
hooks.execute_pre_write(root)?; | ||
} | ||
} |
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.
Do we really need two hooks that are in the same place but different name?
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.
It does seem a little weird, but the idea is that the hooks are semantically different. Long term goal would be that the pre-begin hook runs on all projects on every release, whereas the pre-write hook runs only on projects that actually will write something. Also, if there later exists some hook-able action that occurs before write, the pre-write hook will come after that, but pre-begin will still come before it. Finally, because hooks are all run together, this will give a user to have some control over when their hooks will run in relation to each other.
for proj_id in &self.write.proj_writes { | ||
if let Some((root, hooks)) = args.hooks.get(proj_id) { | ||
hooks.execute_post_tag(root)?; | ||
} | ||
} | ||
|
||
for proj_id in &self.write.proj_writes { | ||
if let Some((root, hooks)) = args.hooks.get(proj_id) { | ||
hooks.execute_post_end(root)?; | ||
} | ||
} |
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.
Same here.
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.
(see above)
This adds additional hooks to the runtime, which (just like the existing
post_write
hook) will not run on a dry run.Hooks added: