Repository navigation
fix: fail tests when srfit calls its own deprecated API - #216
Conversation
Add a pytest filterwarnings rule that turns DeprecationWarnings raised by srfit's own source into errors, and fix the two internal calls it caught: FitRecipe.__init__ (pushFitHook) and initializeRecipe (resultsDictionary). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
||
|
|
||
| def _parse_results_dictionary(results): | ||
| resstr = inputToString(results) |
There was a problem hiding this comment.
I had to add this function so the new deprecation catcher passed
|
@sbillinge ready for review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #216 +/- ##
===========================================
+ Coverage 71.63% 82.71% +11.08%
===========================================
Files 25 27 +2
Lines 3437 4178 +741
===========================================
+ Hits 2462 3456 +994
+ Misses 975 722 -253 🚀 New features to boost your workflow:
|
| dependencies = {file = ["requirements/pip.txt"]} | ||
|
|
||
| [tool.pytest.ini_options] | ||
| # Fail when srfit source calls one of srfit's own deprecated names. |
There was a problem hiding this comment.
This approach seems like technical debt. The kind of tihng Claude loves but us less so. Are we baking code into our core codebase that will be there forever, just to catch stuff that we did to remove old technical debt? Can't we wjust write scripts that do this, run them, catch all the things, fix the, then delete those scripts? Or rather, have Claude do that...?
There was a problem hiding this comment.
@sbillinge I actually requested this from claude because this was/is a problem in diffpy.structure (I think I fixed it but is hasnt been released). An internal call is made to a deprecated function so it emits a deprecation warning that we should've changed which looks bad. I dont view this as technical debt because once all the deprecation work is done and we remove all deprecation markers this can still live here harmlessly. I view this as a safety net to make sure we've changed all internal calls to deprecated functions
There was a problem hiding this comment.
As for your concern about baking code in, I think we are okay because when the deprecated resultsDictionary is removed, the _parse_results_dictionary() won't be used by anything and should also be removed. I see what you're saying about the technical debt here, but I think its worth it. I'd like to think that the person removing this (likely me) will notice this and remove it. I can also add a note to remove that private function when resultsDictionary is removed. If more complex changes are required later we can remove it, but I think the safety net is nice for now.
There was a problem hiding this comment.
ok, I am ok with it. Shall we make an issue to remove the private function explicitly? I will merge this.
There was a problem hiding this comment.
Sure, I can do that
|
@sbillinge replied to your comment^ |
2 participants
Add a pytest filterwarnings that fails tests if diffpy.srfit calls an internally deprecated function. This catches any internal calls to deprecated functions that might have been missed. I caught a couple from this it looks like:
FitRecipe.__init__(pushFitHook) andinitializeRecipe(resultsDictionary).