Skip to content

fix: fail tests when srfit calls its own deprecated API #216

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.

Sign up for GitHub

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
sbillinge merged 2 commits into from
Oct 8, 2026
Merged

fix: fail tests when srfit calls its own deprecated API #216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2ad16e6
fix: fail tests when srfit calls its own deprecated API
cadenmyers13 Oct 8, 2026
7ad4912
add note about removing private function
cadenmyers13 Oct 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions news/fail-on-internal-deprecations.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
**Added:**

* The test suite now fails if srfit's own source code calls one of srfit's
deprecated names, so an internal caller missed during a deprecation is
caught in CI.

**Changed:**

* <news item>

**Deprecated:**

* <news item>

**Removed:**

* <news item>

**Fixed:**

* Creating a ``FitRecipe`` no longer emits a ``DeprecationWarning`` about
``pushFitHook``.
* ``initializeRecipe`` now emits only its own ``DeprecationWarning`` instead of
also warning about ``resultsDictionary``.

**Security:**

* <news item>
6 changes: 6 additions & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ diffpy-srfit = "diffpy.srfit.srfit_app:main"
[tool.setuptools.dynamic]
dependencies = {file = ["requirements/pip.txt"]}

[tool.pytest.ini_options]
# Fail when srfit source calls one of srfit's own deprecated names.

@sbillinge sbillinge Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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...?

@cadenmyers13 cadenmyers13 Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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

@cadenmyers13 cadenmyers13 Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@sbillinge sbillinge Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok, I am ok with it. Shall we make an issue to remove the private function explicitly? I will merge this.

@cadenmyers13 cadenmyers13 Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do that

filterwarnings = [
"error:'diffpy\\.srfit\\.:DeprecationWarning:diffpy\\.srfit",
]

[tool.codespell]
exclude-file = ".codespell/ignore_lines.txt"
ignore-words = ".codespell/ignore_words.txt"
Expand Down
2 changes: 1 addition & 1 deletion src/diffpy/srfit/fitbase/fitrecipe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ def __init__(self, name="fit"):
"""
RecipeOrganizer.__init__(self, name)
self.fithooks = []
self.pushFitHook(PrintFitHook())
self.push_fit_hook(PrintFitHook())
self._restraintlist = []
self._oconstraints = []
self._ready = False
Expand Down
9 changes: 7 additions & 2 deletions src/diffpy/srfit/fitbase/fitresults.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,8 +849,13 @@ def resultsDictionary(results):
dict
The mapping of result names to their string values.
"""
resstr = inputToString(results)
return _parse_results_dictionary(results)


def _parse_results_dictionary(results):
"""Remove this function when when resultsDictionary and
initializeRecipe are removed in 4.0.0."""
resstr = inputToString(results)

@cadenmyers13 cadenmyers13 Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I had to add this function so the new deprecation catcher passed

rx = {
"f": r"[+-]? *(?:\d+(?:\.\d*)?|\.\d+)(?:[eE][+-]?\d+)?",
"n": r"[a-zA-Z_]\w*",
Expand Down Expand Up @@ -892,7 +897,7 @@ def initializeRecipe(recipe, results):
AttributeError
If no results can be found in ``results``.
"""
mpairs = resultsDictionary(results)
mpairs = _parse_results_dictionary(results)
if not mpairs:
raise AttributeError("Cannot find results")

Expand Down
Loading