Make FmfContext read-only#4892
Conversation
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
There was a problem hiding this comment.
Code Review
This pull request refactors FmfContext to inherit from collections.abc.Mapping instead of dict, updating its instantiation across the codebase to use keyword arguments. An issue was identified where the format utility still relies on isinstance(value, dict), which will cause FmfContext objects to be formatted incorrectly. Update _VALUE_FORMATTERS and _format_dict to handle Mapping types.
| def format( | ||
| key: str, | ||
| value: Union[None, float, bool, str, list[Any], dict[Any, Any]] = None, | ||
| value: Union[None, float, bool, str, list[Any], Mapping[Any, Any]] = None, |
There was a problem hiding this comment.
While the type hint is correctly updated to Mapping, the underlying implementation in _format_value still checks for isinstance(value, dict). Since FmfContext is now a Mapping but not a dict, it will not be formatted as a dictionary, but will fall back to its default string representation (<tmt.utils.FmfContext object at ...>). This will likely cause the test_format_value test for fmf context to fail.
To fix this, update _VALUE_FORMATTERS to check for collections.abc.Mapping instead of dict, and update the signature of _format_dict to accept a Mapping.
First step towards unifying this with
fmf.context.Context. Will make an fmf PR equivalent to have a similarMappinginterface (Mapping[str, set[ContextValue]]) and then try to unify the types and constructors.fmf counterpart teemtee/fmf#300