Skip to content
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

Deprecate Diffraction_object, rename to DiffractionObject (new clean PR) #172

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

bobleesj
Copy link
Contributor

It was a direct copy from Diffraction_object to DiffractionObject (no manual modification) except the deprecation warnings/docstrings.

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.17%. Comparing base (b18d485) to head (e3ac7a9).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #172   +/-   ##
=======================================
  Coverage   99.17%   99.17%           
=======================================
  Files           7        7           
  Lines         242      242           
=======================================
  Hits          240      240           
  Misses          2        2           
Files with missing lines Coverage Δ
...ils/scattering_objects/test_diffraction_objects.py 100.00% <100.00%> (ø)

@bobleesj
Copy link
Contributor Author

Once this PR is merged, I can then fix some minor cosmetics issues that you've pointed out in our original PR such as using "the", etc. #165

@@ -461,3 +476,449 @@ def dump(self, filepath, xtype=None):
f.write(f"{key} = {value}\n")
f.write("\n#### start data\n")
np.savetxt(f, data_to_save, delimiter=" ")


class DiffractionObject:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A direct copy of Diffraction_object above. No modification created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing the test.

@bobleesj
Copy link
Contributor Author

@sbillinge ready for review

@sbillinge sbillinge merged commit 9789eac into diffpy:main Nov 14, 2024
5 checks passed
@sbillinge
Copy link
Contributor

nicely done there. As a matter of principle, we could discuss whether to continue to test the deprecated class until it is removed. I think it general we should do so (which means copy-pasting the old test to a new one so that both the old and new one is tested), but I think it is better to just merge this and not worry about it here.... We are just figuring out how to deprecate things and we are the only people using Diffraction_objects so it is not so important.

@bobleesj bobleesj deleted the dobject-depre branch November 15, 2024 19:07
@bobleesj
Copy link
Contributor Author

@sbillinge Yes, good idea to copy and paste existing tests too.

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