Repository navigation
Conversation
ChildRelationship.stringify_param accepted force and never read it, so path(force='yes') returned None for an unrepresentable dict key, the case its own docstring promises to render as '(unrepresentable)'. Adds test_unrepresentable_dict_key_path.
seperman
left a comment
There was a problem hiding this comment.
Static review: #619 Honour force='yes' in stringify_param
Summary: With this change, ChildRelationship.stringify_param uses '(unrepresentable)' when a param has no round-trippable repr and force='yes', and then formats it with param_repr_format. As a result, path(force='yes') no longer returns None for things like custom-object dict keys. That is what the docstrings of path(), get_param_repr() and stringify_param() already promise. On origin/dev, stringify_param accepts force but never reads it, so the bug is real and this fixes it.
Overlap with other PRs
I found no other open, closed or merged PR for this. I searched PR titles and bodies for stringify_param, unrepresentable and force. git merge-tree shows no conflicts with origin/dev or with #618, which is the other open PR that touches deepdiff/model.py.
Security
No security concerns found in static review. The new branch runs only when a caller passes force='yes' explicitly. Every internal caller passes FORCE_DEFAULT = 'fake' (deepdiff/model.py:15), so the text view, Delta, flat rows and path-based exclusion are unchanged.
One note: force='yes' paths are documented as not Python-parsable. If one is passed back into extract() or Delta, _add_to_elements in deepdiff/path.py (line 44 on dev) reads root[(unrepresentable)] as the string key '(unrepresentable)'. The existing set case (root(unrepresentable)) already has this property, so the PR doesn't introduce it.
Correctness
deepdiff/model.py:950-951: the fallback runs before theparam_repr_formatformatting at line 954, so a dict key renders asroot[(unrepresentable)]. The set case goes throughNonSubscriptableIterableRelationship.get_param_repr(deepdiff/model.py:986-988), which skips the formatting and rendersroot(unrepresentable). The bracketed form matches theget_param_reprdocstring ("a formatted param including '(unrepresentable)'"). As the PR description says, this is a convention choice for the maintainer.force=Noneandforce='fake'behave as before (the result staysNone), so plainpath()still returnsNonefor these keys, as documented.- Nit: the
logger.errortext atdeepdiff/model.py:942-945still says "This object will be reported as None", which is no longer accurate whenforce='yes'.
Tests
tests/test_diff_tree.py:124-143tests the custom-object dict-key case. Without the fix,path(force='yes')returnsNone, so the new assertion would fail on the base branch. The test also checks that plainpath()still returnsNone.- Optional: add an assertion that
force='fake'still returnsNone, the waytest_non_subscriptable_iterable_pathdoes. That would pin down the "left alone" behaviour.
Suggestions
- The PR targets
master, but PRs in this repo normally go todev.git merge-treeshows the patch merges cleanly withorigin/dev, so retargeting should be easy.
AI-assisted static review; the code was not executed.
ChildRelationship.stringify_paramtakesforceand never reads it, sopath(force='yes')returns None for the case its own docstring calls out.path()documents it like this:A custom object used as a dictionary key:
reprdoesn't round-trip here, sostringify_paramsetsresult = Noneand returns it whateverforcesays.NonSubscriptableIterableRelationship.get_param_reprdoes implementforce, which is why the set case intest_non_subscriptable_iterable_pathworks and this one never has.Three lines, in
stringify_paramwhereforcealready arrives.One judgement call for you. The result goes through
param_repr_format, so a dict key comes out asroot[(unrepresentable)]while the existing set case isroot(unrepresentable)because it bypasses the format. Brackets seemed right since the key is the part being replaced, but say the word if you'd rather have it raw. I leftforce='fake'alone.Test is
test_unrepresentable_dict_key_path, next to the set-case test. Suite goes 1258 to 1259, with the same 5 failures and 1 error before and after (they are intest_serialization.pyandtest_lfucache.pyand predate this). Reverting onlymodel.pyfails the new test alone.