Skip to content

Honour force='yes' in stringify_param - #619

Open
ckarnell wants to merge 1 commit into
qlustered:masterfrom
ckarnell:fix-stringify-param-force
Open

ckarnell wants to merge 1 commit into
qlustered:masterfrom
ckarnell:fix-stringify-param-force

Conversation

@ckarnell

Copy link
Copy Markdown

ChildRelationship.stringify_param takes force and never reads it, so path(force='yes') returns None for the case its own docstring calls out.

path() documents it like this:

Returns None if the path is not representable as a string. This might be the case ...
because custom objects used as dictionary keys (then there is a path but it's not representable).

:param force: If 'yes':
    Will return a path including '(unrepresentable)' in place of non string-representable parts.

A custom object used as a dictionary key:

class Id:
    def __repr__(self): return '<id>'
    def __hash__(self): return 1
    def __eq__(self, other): return isinstance(other, Id)

k = Id()
change = DeepDiff({k: 10}, {k: 20}, view='tree')['values_changed'][0]
change.path()             # None, correct
change.path(force='yes')  # None, documented as a path with '(unrepresentable)'

repr doesn't round-trip here, so stringify_param sets result = None and returns it whatever force says. NonSubscriptableIterableRelationship.get_param_repr does implement force, which is why the set case in test_non_subscriptable_iterable_path works and this one never has.

Three lines, in stringify_param where force already arrives.

One judgement call for you. The result goes through param_repr_format, so a dict key comes out as root[(unrepresentable)] while the existing set case is root(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 left force='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 in test_serialization.py and test_lfucache.py and predate this). Reverting only model.py fails the new test alone.

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 seperman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the param_repr_format formatting at line 954, so a dict key renders as root[(unrepresentable)]. The set case goes through NonSubscriptableIterableRelationship.get_param_repr (deepdiff/model.py:986-988), which skips the formatting and renders root(unrepresentable). The bracketed form matches the get_param_repr docstring ("a formatted param including '(unrepresentable)'"). As the PR description says, this is a convention choice for the maintainer.
  • force=None and force='fake' behave as before (the result stays None), so plain path() still returns None for these keys, as documented.
  • Nit: the logger.error text at deepdiff/model.py:942-945 still says "This object will be reported as None", which is no longer accurate when force='yes'.

Tests

  • tests/test_diff_tree.py:124-143 tests the custom-object dict-key case. Without the fix, path(force='yes') returns None, so the new assertion would fail on the base branch. The test also checks that plain path() still returns None.
  • Optional: add an assertion that force='fake' still returns None, the way test_non_subscriptable_iterable_path does. That would pin down the "left alone" behaviour.

Suggestions

  • The PR targets master, but PRs in this repo normally go to dev. git merge-tree shows the patch merges cleanly with origin/dev, so retargeting should be easy.

AI-assisted static review; the code was not executed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants