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

[FEATURE] Move block-related logging to a hook #636

Closed
5 tasks
joluj opened this issue Jan 15, 2025 · 2 comments
Closed
5 tasks

[FEATURE] Move block-related logging to a hook #636

joluj opened this issue Jan 15, 2025 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@joluj
Copy link
Contributor

joluj commented Jan 15, 2025

User Story

  1. As a {developer}
  2. I want {have as simple code as possible}
  3. So that {I can read the code}

User Acceptance Criteria

  • Evaluated if it's actually simpler if the block-related generic logging is implemented as a hook.
  • (If so:) Implement the logging as a hook.

Examples

/

Notes

Please evaluate it first if it's a good addition or not.

Definitions of Done

  • A PR has been opened and accepted
  • All user acceptance criteria are met
  • All tests are passing
@joluj joluj added the enhancement New feature or request label Jan 15, 2025
@joluj joluj changed the title [FEATURE] Move block-related logging to a hook. [FEATURE] Move block-related logging to a hook Jan 15, 2025
@TungstnBallon
Copy link
Contributor

@joluj

I see two cases of generic block related logging:

  • execution duration
  • block output

IMO, any logging more specific to the block type's functionality should be left to its executor's doExecute() method.

Logging execution duration

As of now, the hooks' execution order is not sophisticated enough to log execution duration. Currently generic hooks are executed before block specific hooks in the order they are added. Considering #637, the hook logging execution duration would have to be executed after all other hooks, which cannot be guaranteed at this point.

Logging block output

This is currently handled in logBlockResult() which is already generics across block types. Changing that to a hook probably doesn't make the code more or less complex. However, once we add the ability to change/remove hooks, users would be able to replace the default logging. So output logging looks like a great use-case for a generic hook.

@joluj
Copy link
Contributor Author

joluj commented Jan 15, 2025

Okay, thanks for the explanation. Then I'd suggestion closing this issue.

@joluj joluj closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants