Skip to content

Conversation

@tpoliaw
Copy link
Contributor

@tpoliaw tpoliaw commented Nov 4, 2025

The tests make use of the py_run macro which is only available with the
macros feature enabled.

While the py_run macro supports a wider range of requirements, the way
it is used in these tests is straightforward enough that it can be
replaced with a local function to prevent the macros feature needing to
be enabled.

The tests make use of the py_run macro which is only available with the
macros feature enabled.
@tpoliaw tpoliaw force-pushed the ordered-float-test branch from 2804c4b to 662018b Compare November 4, 2025 23:21
@tpoliaw
Copy link
Contributor Author

tpoliaw commented Nov 4, 2025

Probably too late, I just read through the related comments on #5344 and I still think this is ok as a stand-alone change. The tests rely on the macro so requiring it makes some sense.

Would it be better to replace the macro uses so the tests run with no other features enabled or is it still clearer to use the shorthand version?

Replace the py_run macro with a local function offering a simplified but
sufficient functionality
@tpoliaw
Copy link
Contributor Author

tpoliaw commented Nov 5, 2025

Replacing the macro didn't look too bad so is possibly the better approach so the tests aren't skipped unexpectedly.

@tpoliaw tpoliaw changed the title fix: Require macros feature for ordered_float tests fix: Replace macro use in ordered_float tests Nov 7, 2025
@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Nov 7, 2025
@davidhewitt
Copy link
Member

Thanks very much for this! Doing this got me experimenting, IIRC we gated py_run! behind the macros feature to avoid the optional dependencies.

Maybe we can simplify and do something like #5608, and then always allow py_run! macro to be defined?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI-skip-changelog Skip checking changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants