-
Notifications
You must be signed in to change notification settings - Fork 23
chore: deprecate setScatteringFactorTableByType #78
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
Conversation
|
@sbillinge I gave my best attempt at this issue. Here is what I did:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #78 +/- ##
=======================================
Coverage 98.51% 98.52%
=======================================
Files 20 20
Lines 3235 3248 +13
=======================================
+ Hits 3187 3200 +13
Misses 48 48
🚀 New features to boost your workflow:
|
sbillinge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much @zmx27 this is great! I don't have a very good way of checking if the code is good or not, but if you can make the tests as I suggest and they pass I guess it is fine!
Please
- update the error message from "future release" to give the release number of the next major release....
2.0.0or whatever. - make an issue to remove the capability that has been deprecated. We will have to make a milestone for that major release and we can link that issue to it.
- write the tests as requested and make sure they are passing, then let me know. I am not acutally sure how we test the cpp code but figure that out if you would and let m eknow.
Again, gthanks so much.
| bp::object DeprecationWarning = builtins.attr("DeprecationWarning"); | ||
| warnings.attr("warn")( | ||
| std::string("setScatteringFactorTableByType is deprecated; " | ||
| "assign the 'scatteringfactortable' property directly (for example, use SFTNeutron()/SFTXray())."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's give an example of the exact syntax in this help message, as you did in the comment above. But this will be more visible to users.
| dpdfc = self.dpdfc | ||
| dpdfc.setScatteringFactorTableByType("N") | ||
| with self.assertWarns(DeprecationWarning): | ||
| dpdfc.setScatteringFactorTableByType("N") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test is good. However, do we also need to run a test where this is done using the new way? We want to make sure that
- the functionality works with the new syntax (it may be tested elsewhere, but just check)
- the functionality works with the old syntax
- the old syntax triggers the deprecation warning
- the new syntax doesn't trigger the deprecation warning
|
@sbillinge ready for review |
|
Please confirm that you have made an issue in diffpy.srfit to change the call to the new API |
sbillinge
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Please make an issue to remove this method and the tests and I will attach it to a 2.0.0 milestone.
Also, please make an issue in srfiit to change how it is handled in there (and make a PR if possible). Finally, make an issue in diffpy.cmi to check examples for any uses and update them.
Thanks so much.
|
I confirm that an issue has been created in |
Closes #21