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

Bugfix: when 'None' is passed explicitly to _set_num_reasks we don't use the default of '1'. #891

Merged
merged 2 commits into from
Jul 8, 2024

Conversation

JosephCatrambone
Copy link
Contributor

No description provided.

@JosephCatrambone JosephCatrambone marked this pull request as ready for review July 8, 2024 23:28
@@ -235,8 +235,15 @@ def configure(
self._set_tracer(tracer)
self._configure_telemtry(allow_metrics_collection)

def _set_num_reasks(self, num_reasks: Optional[int] = 1):
self._num_reasks = num_reasks
def _set_num_reasks(self, num_reasks: Optional[int]):
Copy link
Contributor

Choose a reason for hiding this comment

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

im no lint/typing expert but surprised theres not a version of pyright that wants Optional[int] = None?

Copy link
Contributor

Choose a reason for hiding this comment

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

ok guess not. lgtm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh I quite detest this function signature. I could do _set_num_reasks(self, num_reasks: int = 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I like num_reasks: Optional[int] = None much more. Good suggestion.

@JosephCatrambone JosephCatrambone merged commit ebaa4de into 0.5.0-dev Jul 8, 2024
12 checks passed
@JosephCatrambone JosephCatrambone deleted the jc/bugfix_set_num_reasks branch July 8, 2024 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants