eval_results decorator replacement
#755
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mih
force-pushed
the
evalresults
branch
5 times, most recently
from
492eee5
to
6aa4eea
Compare
mih
force-pushed
the
evalresults
branch
3 times, most recently
from
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. |
mih
force-pushed
the
evalresults
branch
2 times, most recently
from
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`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.