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

Allow unlimited tries #95

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Allow unlimited tries #95

wants to merge 4 commits into from

Conversation

mvastola
Copy link

@mvastola mvastola commented Dec 30, 2020

So I'm dumb and am only now realizing a PR (#20) already exists for this, but as it's 4+ years old and is based on an older version of the code, maybe this is worth it anyway.

As noted in #20, implementing this requires the use of Enumerators because the ExponentialBackoff class currently produces an array, which is no longer feasible.

Thoughts/comments/criticism very welcome.

Note: Fixes #94. Obviously fixes #20 as well.

Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Error: Unsupported Ruby version 2.0 found in `TargetRubyVersion` parameter (i...
Error: Unsupported Ruby version 2.0 found in `TargetRubyVersion` parameter (in .rubocop.yml). 2.0-compatible analysis was dropped after version 0.50.
Supported versions: 2.1, 2.2, 2.3, 2.4, 2.5

Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Error: Unsupported Ruby version 2.0 found in `TargetRubyVersion` parameter (i...
Error: Unsupported Ruby version 2.0 found in `TargetRubyVersion` parameter (in .rubocop.yml). 2.0-compatible analysis was dropped after version 0.50.
Supported versions: 2.1, 2.2, 2.3, 2.4, 2.5

@mvastola
Copy link
Author

Update: Apparently this is failing now due to the Gemfile.lock so I need to unravel that. Also, hound is failing because it is invoking rubocop instead of bundle exec rubocop so it's running against a newer version. But newer versions encourage constructs (i.e. safe navigation operator) not supported in all of 2.x. Will see if I can get this working. (Complicating matters is that 2.0 will no longer compile on my version of Ubuntu.)

As this gem supports all ruby versions >= 2.0, fix rubocop version to 0.50 (the last version supporting ruby 2.0).

Also fix all auto-correctable and/or superficial cops.

Remove `Gemfile.lock` from `.gitignore` (per kamui#94) and commit working lockfile.
Tests included.

Some refactoring required to implement this as current implementation's ExponentialBackoff class produced an array, and infinite retries are incompatible with a finite # of values. Changed code to support/use (lazy) enumerators instead.
Causing warnings due to conditional logic in gemspec.
Also don't seem to be used.
Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

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

Some files could not be reviewed due to errors:

Error: Unsupported Ruby version 2.0 found in `TargetRubyVersion` parameter (i...
Error: Unsupported Ruby version 2.0 found in `TargetRubyVersion` parameter (in .rubocop.yml). 2.0-compatible analysis was dropped after version 0.50.
Supported versions: 2.1, 2.2, 2.3, 2.4, 2.5

- Rubocop
  - Moved rubocop checks into travis and disabled hound
    - Hound did not support rubocop 0.50.x, which is the last version to
    support Ruby v2
  - Condensed/simplified .rubocop.yml
    - Corrected easily fixed cops
- Gemfile
  - Added extra constraints due to encountered bugs
  - Added pry enhancements
    - TODO: add to gemspec
- Rake
  - Added Rakefile with tasks to simplify testing (both locally and on
  travis)
- Travis
  - Upgraded OS to use Ubuntu xenial rather than trusty (end of life)
  - Script now runs bundle exec rake rather than rspec directly
@kamui
Copy link
Owner

kamui commented Jul 29, 2021

@mvastola Can I ask what's the use case for needing or wanting unlimited retries? This change will change the API enough that it would likely need a major release. Unlimited tries is not something I'd ever use, especially with exponential backoff as that interval could get so long between tries. When would you not want it to eventually give up? It just seems like it could be dangerous to retry infinitely.

@Nakilon
Copy link

Nakilon commented Jul 26, 2022

When would you not want it to eventually give up?

Any service depending on another service that may go down for undefined amount of time. Like when some websites break the API and aren't hurrying to fix it and my bots deployed once in a year would like to wait until it works again instead of halting and doubling their offline time by waiting for me to restart them.

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.

Request: Remove Gemfile.lock from .gitignore
3 participants