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

Performance improvement #1

Merged
merged 3 commits into from Jan 5, 2023
Merged

Performance improvement #1

merged 3 commits into from Jan 5, 2023

Conversation

Agade09
Copy link

@Agade09 Agade09 commented Jan 5, 2023

I was having significant overhead from inspect.getcallargs() in _genericPyDirectInputChecks. I don't understand why such an expensive function is necessary in this case as a simple if statement seems to achieve very similar functionality.

@ReggX
Copy link
Owner

ReggX commented Jan 5, 2023

I like the thought behind this, but it comes with a problem: It breaks code that has been using _pause as positional argument!

I'm okay with adding

if '_pause' in kwargs:
    _pause = kwargs['_pause']
else:
    funcArgs: dict[str, Any] = (
        inspect.getcallargs(wrappedFunction, *args, **kwargs)
    )
   _pause = funcArgs.get("_pause")
_handlePause(_pause)

That will skip the getcallargs call if the _pause keyword argument is present while keeping backwards compatibility for positional argument use.

Please amend your code and I will merge it into master.

@ReggX ReggX self-assigned this Jan 5, 2023
@ReggX ReggX merged commit f1b2cbe into ReggX:master Jan 5, 2023
@Agade09
Copy link
Author

Agade09 commented Jan 5, 2023

There is an indentation error. A space is missing before the last _pause. How best to resolve this? Shall i make a new pull request? Do you fix it on your side?

@ReggX
Copy link
Owner

ReggX commented Jan 5, 2023

I fixed it when I bumped the version number, no worries.

Congratulations on your first contribution. 👏

@ReggX ReggX added the performance Affects library performance label Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Affects library performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants