Skip to content

Remove unused test util functions for comparing two dicts, use __eq__ to compare DiffractionObjects #214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

bobleesj
Copy link
Contributor

No description provided.

Copy link

Warning! No news item is found for this PR. If this is a user-facing change/feature/fix,
please add a news item by copying the format from news/TEMPLATE.rst.

# setattr(diffraction_object1, attribute, inputs1[i])
# setattr(diffraction_object2, attribute, inputs2[i])
print(dicts_equal(diffraction_object1.__dict__, diffraction_object2.__dict__), expected)
assert dicts_equal(diffraction_object1.__dict__, diffraction_object2.__dict__) == expected
Copy link
Contributor Author

@bobleesj bobleesj Dec 12, 2024

Choose a reason for hiding this comment

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

Since we will be introducing an unique uuid via _id, this test will fail. But rather we use __eq__ method to compare two objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bobleesj yes, this is much cleaner. A sure sign that our code and API is improving, don't you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

absolutely

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.68%. Comparing base (77c0b9e) to head (73b2f46).
Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #214      +/-   ##
==========================================
+ Coverage   96.54%   99.68%   +3.14%     
==========================================
  Files           8        8              
  Lines         347      316      -31     
==========================================
- Hits          335      315      -20     
+ Misses         12        1      -11     
Files with missing lines Coverage Δ
tests/test_diffraction_objects.py 100.00% <100.00%> (+11.57%) ⬆️

@@ -8,43 +8,6 @@

from diffpy.utils.diffraction_objects import XQUANTITIES, DiffractionObject


def compare_dicts(dict1, dict2):
Copy link
Contributor Author

@bobleesj bobleesj Dec 12, 2024

Choose a reason for hiding this comment

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

These functions are no longer used in our test code - We. may use the DeepDiff library instead to compare two dicts as implemented in our previous PR:

@pytest.mark.parametrize("inputs, expected", tc_params)
def test_constructor(inputs, expected):
    actual_do = DiffractionObject(**inputs)
    diff = DeepDiff(actual_do.__dict__, expected, ignore_order=True, significant_digits=13)
    assert diff == {}

Copy link
Contributor Author

@bobleesj bobleesj left a comment

Choose a reason for hiding this comment

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

While I was waiting for other PRs to be merged, I was reading the code and found these potential areas of improvement. Please let me know! @sbillinge

@bobleesj bobleesj marked this pull request as ready for review December 12, 2024 03:00
Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

yes, thanks for this @bobleesj

# setattr(diffraction_object1, attribute, inputs1[i])
# setattr(diffraction_object2, attribute, inputs2[i])
print(dicts_equal(diffraction_object1.__dict__, diffraction_object2.__dict__), expected)
assert dicts_equal(diffraction_object1.__dict__, diffraction_object2.__dict__) == expected
Copy link
Contributor

Choose a reason for hiding this comment

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

@bobleesj yes, this is much cleaner. A sure sign that our code and API is improving, don't you think?

@sbillinge sbillinge merged commit fce1a32 into diffpy:main Dec 12, 2024
4 of 5 checks passed
@bobleesj bobleesj deleted the test-func-std branch December 12, 2024 17:42
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