-
Notifications
You must be signed in to change notification settings - Fork 9
Add watchdog_interval method
#11
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
Add watchdog_interval method
#11
Conversation
This adds a `watchdog_interval` method as well as tests for it and for `watchdog?` (which previously had no tests). I also took the liberty of revising the docs for `watchdog?` to (I hope) be a little clearer. Fixes agis#6.
Running Rubocop printed out a huge number of new warnings and errors. For the most part, I just silenced them since this gem has been pretty stable and I assume you don't want to modify things just to match newer recommendations. I did add `required_ruby_version` to the gemspec, though, since that just seems like a good idea. It's set to 2.3.0, based on the GitHub Actions CI job.
.rubocop.yml
Outdated
| AllCops: | ||
| TargetRubyVersion: 2.2 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given how many cops I had to turn off in this PR, it might be better to disable them all here and explicitly turn on the ones you want. I’m not sure which you want, though, so I just turned off all the new and/or error-triggering ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the issue is that rubocop was never pinned to a specific version. I'd suggest to just disregard all the offenses, given that it's not a requirement in the CI, and just revert this change here, since it's out of context of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, would it be good to at least remove this Rubocop config file, then? It seems a little confusing to have an unusable .rubocop.yml file here.
|
Just checking in on this, since it's been sitting for a year. Anything you need from me to land this, @agis? |
agis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the radio silence. Left some comments, thanks for this!
lib/sd_notify.rb
Outdated
| wd_usec = Integer(ENV["WATCHDOG_USEC"]) | ||
| wd_usec > 0 ? wd_usec / 1e6 : 0.0 | ||
| rescue StandardError | ||
| 0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a clearer API if we returned nil when the value isn't set. Also I'd prefer to limit this rescue to the place it's intended for, i.e
def self.watchdog_interval
wd_usec = Integer(ENV["WATCHDOG_USEC"]) rescue nil
wd_usec && wd_usec / 1e6
endWDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a clearer API if we returned nil when the value isn't set.
Sure. (Just for background, I usually try to avoid returning a different types as a general practice so the JIT can keep methods optimized. This probably isn’t a method that’s going to be on a hot path, though, so that’s not a big deal here.)
.rubocop.yml
Outdated
| AllCops: | ||
| TargetRubyVersion: 2.2 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the issue is that rubocop was never pinned to a specific version. I'd suggest to just disregard all the offenses, given that it's not a requirement in the CI, and just revert this change here, since it's out of context of this PR.
sd_notify.gemspec
Outdated
| s.files = ["lib/sd_notify.rb", "LICENSE", "README.md", "CHANGELOG.md"] | ||
| s.homepage = "https://github.com/agis/ruby-sdnotify" | ||
| s.license = "MIT" | ||
| s.required_ruby_version = ">= 2.2.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale behind this requirement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the PR description:
When I ran Rubocop, I got a huge number of errors, mostly from cops that are probably new since this gem last received a major update. I went ahead and turned most of them off in the config, although I did add
required_ruby_versionto the gemspec file since that seemed like a good idea. I set the value (>= 2.3.0>= 2.2.0) based on the GitHub Actions CI job. Let me know if I should change or remove it.
That is, Rubocop required that I add required_ruby_version or disable that cop. It seemed more reasonable in this case to add it to the gemspec and ask you if you wanted a different value.
From your other comments, it sounds like you'd rather just ignore Rubocop for this repo, though, so I can pull this back out if that’s what you’d like. (Is that what you’d like?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks for elaborating. In that case, yes, let's remove the rubocop config completely.
test/sd_notify_test.rb
Outdated
| ENV["WATCHDOG_PID"] = $$.to_s | ||
| setup_socket | ||
|
|
||
| assert_equal(true, SdNotify.watchdog?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be simplified to `assert SdNotify.watchdog?
The existing Rubocop config is set up for an unknown version of Rubocop from several years ago that was never pinned. The current rules Rubocop suggests are undesired, so this just removes it instead.
The `ruby/setup-ruby` action does not support Ruby 2.2 on Ubuntu 22+ (see ruby/setup-ruby#496) and GitHub Actions no longer supports older versions of Ubuntu, so we can no longer test Ruby 2.2 (at least not without a lot of extra work!).
98cada3 to
dbf0dfd
Compare
dbf0dfd to
9ff9cdc
Compare
a40c59b to
20b1483
Compare
|
@agis OK, I think I’ve made all the changes you asked for and this is ready for re-review. Since this PR was originally written, GitHub dropped support for Ubuntu 20.04, so I had to update the CI config a bit — the “old” tests run in Ubuntu 22.04, and use Ruby 2.1 since Ruby 2.2 is no longer supported by the |
|
Thanks! |
This adds a new
watchdog_intervalmethod that returns the current watchdog interval in seconds (or0if no interval is set). Fixes #6.A few notes here:
I tried to keep this simple by just adding a
watchdog_intervalmethod and not changing any other code. That said, it might be convenient ifwatchdog?returnedfalseif disabled and the interval (instead oftrue) if enabled. That would work fine for most people (since floats are truthy), but might be subtly breaking if someone is literally comparing the return value totrue.Revised the docs for
watchdog?a bit to hopefully make them clearer for users who aren’t already deeply familiar withsd_notify, instead of focusing on under-the-hood bits like environment variables. Happy to change this back if you don’t like that.Added tests for
watchdog?since there weren’t any.Added an example of using watchdog to the README. It’s a little complex! Let me know if you have ideas for making it simpler or clearer, or if I should remove it.
When I ran Rubocop, I got a huge number of errors, mostly from cops that are probably new since this gem last received a major update. I went ahead and turned most of them off in the config, although I did add
required_ruby_versionto the gemspec file since that seemed like a good idea. I set the value (>= 2.3.0>= 2.2.0) based on the GitHub Actions CI job. Let me know if I should change or remove it.