Skip to content

Conversation

@curzolapierre
Copy link
Member

  • Add a changelog entry in CHANGELOG.md

@curzolapierre curzolapierre self-assigned this May 6, 2025
Copy link
Member

@leo-scalingo leo-scalingo left a comment

Choose a reason for hiding this comment

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

So if I do:

r = NewRetrier(WithoutMaxAttempt())

It will be set to 5, not really logical unfortunately

@curzolapierre
Copy link
Member Author

It will be set to 5, not really logical unfortunately

Oh indeed, this case is not covered in tests.

So we face to the issue of default value here.
Or what about returning an error in this particular case

What would be the use case of retrier without MaxAttempts?
Does this mean we are expecting an infinite retry? Or we do necessarily want a limit, in which case we need to specify it

@curzolapierre curzolapierre requested a review from leo-scalingo May 6, 2025 07:52
@leo-scalingo
Copy link
Member

Actually I think it's better to

  • Have good defaults
  • Be explicit in the API, which means, no side effect

The condition you're adding is a kind of side effect which may lead to situations which are difficult to understood without reading the doc/code of the library.

So to me the question is:

  • Is the current default un-practical and we want to change it?
  • What would be the ideal explicit API for you?

Copy link
Member

@leo-scalingo leo-scalingo left a comment

Choose a reason for hiding this comment

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

See above comment

@EtienneM
Copy link
Member

EtienneM commented Sep 5, 2025

@curzolapierre I feel like you forgot about this one. Can you revive this PR or close it?

@EtienneM
Copy link
Member

EtienneM commented Nov 6, 2025

@curzolapierre is it still under your radar?

@EtienneM EtienneM closed this Nov 25, 2025
@EtienneM EtienneM deleted the feat/retry/MaxDuration_should_ignore_MaxAttempts_default branch November 25, 2025 14:35
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.

4 participants