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

Fix sample code for Timeout overload #1109

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

prodehghan
Copy link
Contributor

The overload takes a Func<TimeSpan>, and it is assumed that we already have a lambda variable called `myTimeoutProvider, but the variable is incorrectly passed to the overload.

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch

The overload takes a `Func<TimeSpan>`, and it is assumed that we already have a lambda variable called `myTimeoutProvider, but the variable is incorrectly passed to the overload.
@@ -619,7 +619,7 @@ Policy

// Configure variable timeout via a func provider.
Policy
.Timeout(() => myTimeoutProvider)) // Func<TimeSpan> myTimeoutProvider
.Timeout(myTimeoutProvider)) // Func<TimeSpan> myTimeoutProvider
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, it could also be this:

Suggested change
.Timeout(myTimeoutProvider)) // Func<TimeSpan> myTimeoutProvider
.Timeout(() => myTimeoutProvider())) // Func<TimeSpan> myTimeoutProvider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but why someone would need to do this? 😊

Copy link
Member

Choose a reason for hiding this comment

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

IMHO it makes the intent clearer to the reader outside of an IDE (i.e. in our documentation) that it's a delegate and not simple variable.

Copy link
Contributor Author

@prodehghan prodehghan Apr 11, 2023

Choose a reason for hiding this comment

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

I also thought of that, but that's why there is a comment in front of the code. It is not always possible (or good) to make everything super easy. The reader already took the time to read the README file to that point and already looked at samples. At this point, there are four different hints for the reader: 1- This sample should have something new compared to the two samples above it, 2- The comment above the code says "via a func provider", 3- the variable name ends with Provider, 4- The comment in front states the variable type.

No one can help someone that just wants to copy a code snippet that he/she found, blindly, without paying attention.

Sorry for the long comment 😋. I hope I was able to deliver my point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And the library is already complicated enough, that not paying attention will not help the reader.

@martincostello martincostello merged commit 58efa86 into App-vNext:main Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants