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

Move result size check one level higher #1714

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Conversation

khk-globus
Copy link
Contributor

No functional change but move result length check one level higher in the call stack. There's no sense in passing the size limit. _call_user_function's purview is to invoke the function. Checking the result validity is for the higher-level helper.

While in _call_user_function, make IDE slightly happier by not shadowing the module variable.

Type of change

  • Code maintenance/cleanup

@khk-globus khk-globus added no-news-is-good-news This change does not require a news file quick-review Review of this should be quick and easy labels Nov 13, 2024
No functional change but move result length check one level higher in the call
stack.  There's no sense in passing the size limit.  `_call_user_function`'s
purview is to invoke the function.  Checking the result validity is for the
higher-level helper.

While in `_call_user_function`, make IDE slightly happier by not shadowing the
module variable.
@khk-globus khk-globus force-pushed the minor_execute_task_refactor branch from 355c9a6 to c641420 Compare November 13, 2024 12:59
Copy link
Collaborator

@yadudoc yadudoc left a comment

Choose a reason for hiding this comment

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

Nice! This focuses _call_user_function to a single responsibility.

@khk-globus khk-globus merged commit 43d4ff9 into main Nov 13, 2024
17 checks passed
@khk-globus khk-globus deleted the minor_execute_task_refactor branch November 13, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-news-is-good-news This change does not require a news file quick-review Review of this should be quick and easy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants