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

Make sys.argspec handle functions with annotations #61080

Merged
merged 17 commits into from
Feb 24, 2022

Conversation

msteed
Copy link
Contributor

@msteed msteed commented Oct 19, 2021

What does this PR do?

Makes sys.argspec not die on functions that use annotations

What issues does this PR fix or reference?

Fixes: #48735

Previous Behavior

Calling sys.argspec on a salt function that used annotations failed with

ValueError: Function has keyword-only arguments or annotations, use getfullargspec() API which can support them

New Behavior

Calling sys.argspec on a salt function with annotations works (annotations are ignored)

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

- always use inspect.signature in salt.utils.args.get_function_argspec()
  (no more getfullargspec)
- add a test

refs saltstack#48735
@msteed msteed requested a review from a team as a code owner October 19, 2021 20:18
@msteed msteed requested review from xeacott and removed request for a team October 19, 2021 20:18
@msteed
Copy link
Contributor Author

msteed commented Oct 20, 2021

Might also fix #55499

@msteed
Copy link
Contributor Author

msteed commented Feb 1, 2022

@xeacott - can I get a review please?

@xeacott
Copy link
Contributor

xeacott commented Feb 9, 2022

Lint failed for some reason on this... I'm not too familiar with the change itself however the code looks good, test looks good and changelog is there. If you feel that an integration test or another unit test would be helpful here, maybe to test a negative outcome or ensure that pytest doesn't raise a ValueError, that would be valuable.

@msteed
Copy link
Contributor Author

msteed commented Feb 9, 2022

@xeacott - I already added a test to cover the previously failing case. Is there something else I should be testing?

I know lint has passed on this PR before. I'll update from master and let it run again.

@xeacott
Copy link
Contributor

xeacott commented Feb 10, 2022

@msteed Oh thought that test file was inside pytest. Would you be able to covert it? Previously that's been an ask for smaller files (to convert an older test file to pytest) from the core team.

@msteed
Copy link
Contributor Author

msteed commented Feb 10, 2022

Converted ✔️

@xeacott
Copy link
Contributor

xeacott commented Feb 14, 2022

re-run full all

@msteed
Copy link
Contributor Author

msteed commented Feb 17, 2022

Failing checks seem unrelated to these changes (failing to create VMs)

@s0undt3ch s0undt3ch added the Phosphorus v3005.0 Release code name and version label Feb 17, 2022
@dmurphy18
Copy link
Contributor

@msteed We are all waiting for the amazon and windows tests to work, SRE looking into it

@Ch3LL Ch3LL merged commit f0e6a2d into saltstack:master Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Phosphorus v3005.0 Release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getfullargspec and annotations
6 participants