Skip to content

fix: correct reversed arguments in warnings.warn call#120

Open
nishantharkut wants to merge 2 commits intoioos:mainfrom
nishantharkut:fix/warnings-warn-args
Open

fix: correct reversed arguments in warnings.warn call#120
nishantharkut wants to merge 2 commits intoioos:mainfrom
nishantharkut:fix/warnings-warn-args

Conversation

@nishantharkut
Copy link

Summary

This PR fixes the deprecated compatibility wrapper so warning emission works correctly.

Problem

The warnings.warn positional arguments were reversed. That makes Python treat a string as the warning category, which raises:
TypeError: category must be a Warning subclass, not 'str'.

Changes

  1. Put the warning message in the first argument position.
  2. Pass DeprecationWarning as the category.
  3. Add stacklevel=2 so the warning location points to caller code.
  4. Add a focused regression test for warning category, message text, and caller location behavior.

Validation

  1. Utility tests pass locally.
  2. Lint checks pass locally for the changed files.

…ogy wrapper

Swap the warning message and category arguments in the utils compatibility wrapper so DeprecationWarning is emitted correctly. Add stacklevel=2 so the warning points to user call sites, and add a regression test that checks warning category, message content, and caller filename.
Copilot AI review requested due to automatic review settings March 17, 2026 06:13
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a deprecated compatibility wrapper so that deprecation warnings are emitted correctly (message/category order and caller location), and adds a regression test to validate the warning behavior.

Changes:

  • Fix warnings.warn call argument order and add stacklevel=2 for correct caller attribution.
  • Add a regression test to validate warning category, message content, and reported filename.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
xarray_subset_grid/utils.py Corrects warnings.warn usage in the deprecated assign_ugrid_topology wrapper and sets stacklevel=2.
tests/test_utils.py Adds a focused test to assert the wrapper emits a DeprecationWarning and points at the caller location.
Comments suppressed due to low confidence (1)

xarray_subset_grid/utils.py:108

  • from .grids.ugrid import assign_ugrid_topology rebinds the local name to a different function with the same name as this deprecated wrapper. That works, but it makes stack traces/debugging confusing and is easy to misread as recursion. Consider importing with an alias (e.g., _assign_ugrid_topology) and returning that instead.
        stacklevel=2,
    )
    from .grids.ugrid import assign_ugrid_topology

    return assign_ugrid_topology(*args, **kwargs)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 100 to 102
"The function `assign_grid_topology` has been moved to the "
"`grids.ugrid` module. It will not be able to be called from "
"the utils `module` in the future.",
"The function `assign_grid_topology` has been moved to the "
"`grids.ugrid` module. It will not be able to be called from "
"the utils `module` in the future.",
DeprecationWarning,
…essage

Apply Copilot review suggestions:
- Fix function name in warning message: change assign_grid_topology to assign_ugrid_topology (the correct function name in the repo)
- Remove backticks around second occurrence of 'module' (plain English word, not code reference)
- Use keyword argument category=DeprecationWarning instead of positional argument
- Update test assertion to match corrected message
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