Skip to content
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

Allow dimension-checking of multiple return values #435

Merged
merged 9 commits into from
Aug 21, 2023
Merged

Allow dimension-checking of multiple return values #435

merged 9 commits into from
Aug 21, 2023

Conversation

db434
Copy link
Contributor

@db434 db434 commented Aug 4, 2023

Extend the @returns decorator so it can handle functions with multiple return values.

Example:

>>> @returns(length, length/time**2)
... def f(a, v):
...     return a * v, v / a
...
>>> res = f(a= 2 * u.s, v = 3 * u.m/u.s)
>>> print(*res)
6 m 1.5 m/s**2

I think I've followed everything in CONTRIBUTING, but please let me know if I've missed anything.

@neutrinoceros neutrinoceros added the enhancement New feature or request label Aug 4, 2023
@neutrinoceros
Copy link
Member

This seems like a good idea ! I don't have time to review in detail right now but I'll be back very shortly. In the mean time, I suggest updating this decorator's documentation to reflect the new possibility. Thanks for your work !

@db434
Copy link
Contributor Author

db434 commented Aug 9, 2023

Thanks for taking a look.

In the mean time, I suggest updating this decorator's documentation to reflect the new possibility.

I think I've done this already - is there anywhere else that you would suggest changing?

@neutrinoceros
Copy link
Member

You've updated the docstring (that's great !), but I'm talking about the narrative documentation. Namely, I'd expect some update to docs/usage.rst

@neutrinoceros
Copy link
Member

Thank you for following through ! automated testing seems to have caught some syntax error in the docs. I suggest you try to run tox locally to make sure everything is okay, because currently we need to manually approve your tests to run every time you push (this is a security measure against bad agents, it only applies to new contributors !).

@db434
Copy link
Contributor Author

db434 commented Aug 10, 2023

Sorry for this CI spam - I've struggled to get all of the tests running locally. Hopefully this is the last time..!

Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

This looks great ! thank you so much for working on this and adding tests ! I found a subtle way your change can break backward compatibility for some users, and provided some indications on how to avoid that. Don't hesitate if you have any questions about that

docs/usage.rst Outdated Show resolved Hide resolved
unyt/tests/test_unyt_testing.py Outdated Show resolved Hide resolved
unyt/dimensions.py Outdated Show resolved Hide resolved
unyt/dimensions.py Outdated Show resolved Hide resolved
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

Awesome job ! The only missing part I can think of would be a couple tests that the deprecated API behaves as expected, but I've tested it manually and I'd be okay to take the PR as is and add the tests later. I'll defer merging for a week or two so as to give other maintainers a chance to weigh in.

@db434
Copy link
Contributor Author

db434 commented Aug 11, 2023

I believe those tests are here. Is there anything else you're looking for?

Thanks for your patience with this!

@neutrinoceros
Copy link
Member

Oh I somehow missed them, even better ! I have no further request.

@jzuhone jzuhone merged commit 37d4f27 into yt-project:main Aug 21, 2023
8 checks passed
@db434 db434 deleted the multiple_return_values branch August 21, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants