-
Notifications
You must be signed in to change notification settings - Fork 9
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
eval_results
decorator replacement
#755
Conversation
492eee5
to
6aa4eea
Compare
02541a9
to
add19a3
Compare
From my POV this has reached a sensible milestone. I will now evaluate the new possibilities in the context of https://hub.datalad.org/datalad/dlcmd Once proven, this can be merged. |
7c01990
to
98e7949
Compare
Previously the entrypoint for the patch was `_execute_command_()`. But now we aim to refactor the whole thing.
Previously, kwargs for the actual command were handled semi-separately from kwargs for result handling. However, I see no benefits, only added complexity.
`eval_func()` and its dependencies are a spaghetti monster. This commit introduces a `ResultHandler`, a class that provides functionality to handle results produced by a command execution. This idea is to factor out all result processing from result generation code. Moreover, a caller should eventually be able to provision an alternative result handler with different implementations for filtering, and rendering results.
The aim is to jointly refactor them.
And `LegacyResultHandler` as its only implementation for now.
This is now done by a configurabled, dedicated handler class.
This replaces the initial implementation that would add yet another kwarg to each command. There is no real usecase for switching the result handler on a call-by-call basis. This new implementation follows the pattern found in libraries (like matplotlib) to set handlers or backends using top-level helper functions.
This anticipates a future move to `hatch` for project and environment management.
The helper `_execute_command_()` is not the place to decide on the fate of a results, or to impose any constraints on how it can be used. If a logger "does not serialize, or pollutes the output", this is only relevant, when a result needs to be serialized, or printed. Any sensible implementation of such functionality will need to have means to deal with this problem (a logger is not the only source of complications). Removing it here is therefore premature.
Otherwise it does not affect the remaining patches, like it should. One example is the `configuration` patch, which essentially replaces an entire command implementation.
This will eventually fix datalad#397
This will eventually yield a near drop-in replacement for the legacy `ConfigManager`.
I think this can be closed now. The next iteration along these lines is proposed for inclusion into datalad-core: datalad/datalad-core#25 It is part of a more radical change that replaces all of the legacy (and also next's) approach to command implementations with a single decorator. |
No description provided.