Diff improvements

[codecov[bot]]

Codecov Report

Merging #7 (bdbc875) into dev (7ed52fe) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##              dev       #7   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files          31       31           
  Lines       30938    30951   +13     
=======================================
+ Hits        30914    30927   +13     
  Misses         24       24           

Impacted Files	Coverage Δ
hikaru/__init__.py	100.00% <ø> (ø)
hikaru/meta.py	98.34% <100.00%> (+0.05%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7ed52fe...bdbc875. Read the comment docs.

[aantn]

Hi, Thank you for the comments. I'll try to address everything below by category but let me know if I've missed anything:

1. Docs: I've fixed the docs as requested. I'm not sure what command you're using to generate the docs so I didn't actually generate html with my changes. Let me know if I have any mistakes there that need fixing or feel free to fix them yourself. (In general, I'm not attached to my code, so you can modify anything I submit as you see fit. Don't mind at all.)
2. Regarding diff vs _diff: I added _diff because it makes the implementation much simpler. You can't recursively call attr.diff() without first checking attr's type because diff() only exists on subclasses of HikaruBase. For example, "some string".diff() is clearly illegal, but HikaruBase._diff("some string") is legal. So adding _diff() simplifies the implementation and removes duplicate code. (One symptom of this: in your original diff() function you had duplicate code that explicitly checked for int/float classes in two places, but in the new version it appears only once.)
3. TYPE_CHANGED vs INCOMPATIBLE: my rationale was that INCOMPATIBLE is returned when the high-level call that the user made to diff() is illegal because the two objects have a different type. On the other hand, if the internal diff contains a type change then TYPE_CHANGED is returned. As you point out, I'm not sure if there are actual cases where this is legal. Does the swagger file / openapi have a concept of Union types? (By the way, you might want to consider generating pydantic subclasses instead of dataclasses in a future version of hikaru. Pydantic is a python library that enforces type annotations and prevents illegal assignments.) Regardless, I'll update the code to whatever behaviour you like here. I don't think it matters one way or another for my usecase, so let me know how I should handle it.
4. Report messages: I changed the text to "Added:" and "Removed:" as that applies to both attributes and dictionary/list values. Is that OK?

I think fixed the other issues, except for adding a unit test that object_at_path() plays well with DiffDetail.path. I'll do that later today or tomorrow. Let me know what you think of all the other changes.

On an unrelated note, I think you still have an issue with setup.py(). Also, can you add a doc specifying how you run the unittests? I mucked around with pytest a little, but I had to change PYTHONPATH to get it to work. How do you run it?

[aantn]

I've added a test for checking object_at_path and the test passes successfully.

On the other hand, the following test (not currently committed) does not pass because object_at_path can't traverse dictionary keys.

def test122():
    """
    test that you can run object_at_path on the path returned by diff()
    """
    pod = Pod(
        spec=PodSpec(
            containers=[
                Container(name="a", resources=ResourceRequirements(limits={"foo": "bar"}))
            ]
        )
    )
    pod2 = copy.deepcopy(pod)
    pod2.spec.containers[0].resources.limits["foo"] = "blah"
    diff = pod.diff(pod2)
    print(diff)
    print("object at path is", pod.object_at_path(diff[0].path))

Lmk how you want to handle this.

[haxsaw]

Hi-- Thanks for the changes; they look good.

I understand your point re: diff() vs _diff(); that works well.

I also get what you're saying in regards to incompatible vs type changed. I think I'd prefer consistent error reporting regardless of where the error occurred if it's the same kind of condition generating the error. I'll deal with that change, so don't trouble yourself there.

I like your solution for the changing the use of 'Key' when an attribute was being discussed; that works for me.

Lastly, I've been thinking about the removal of 'attr' from DiffDetail, and I think I'm going to put it back, but perhaps as a property. I know you don't use it, but there have been a few hundred downloads of Hikaru since I announced it and I don't want to break anyone's code more than necessary. The change of import is minor compared to the number of times someone might look at an attr.

[aantn]

Awesome, thank you. I'm going to open another ticket re. adding support to object_at_path for dictionary lookups.