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

Issue #572: make kernel warn configurable #579

Merged

Conversation

ColinDKelley
Copy link
Collaborator

@ColinDKelley ColinDKelley commented Feb 22, 2024

  • Makes the usage of Kernel#warn configurable through Listen.adapter_warn_behavior =.
  • Adds this documentation to the README on that feature:

Adapter Warnings

If listen is having trouble with the underlying adapter, it will display warnings with Kernel#warn by default,
which in turn writes to STDERR.
Sometimes this is not desirable, for example in an environment where STDERR is ignored.
For these reasons, the behavior can be configured using Listen.adapter_warn_behavior =:

Listen.adapter_warn_behavior = :warn   # default (true means the same)
Listen.adapter_warn_behavior = :log    # send to logger.warn
Listen.adapter_warn_behavior = :silent # suppress all adapter warnings (nil or false mean the same)

Also there are some cases where specific warnings are not helpful.
For example, if you are using the polling adapter--and expect to--you can suppress the warning about it
by providing a callable object like a lambda or proc that determines the behavior based on the message:

Listen.adapter_warn_behavior = ->(message) do
  case message
  when /Listen will be polling for changes/
    :silent
  when /directory is already being watched/
    :log
  else
    :warn
  end
end

Copy link

@houndci-bot houndci-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: RuboCop found unknown Ruby version 3.0 in `TargetRubyVersion` paramete...
Error: RuboCop found unknown Ruby version 3.0 in `TargetRubyVersion` parameter (in .rubocop.yml).
Supported versions: 2.4, 2.5, 2.6, 2.7, 2.8

@AlexB52
Copy link
Contributor

AlexB52 commented Feb 22, 2024

Question: Indirect use of Listen

Having access to Listen gem directly allows configuration but what about people using Listen indirectly through guard for example. I'm not super familiar with guard, would people need to be able to silence these or would it be the gem using Listen to make a port for Listen configuration.

@ColinDKelley
Copy link
Collaborator Author

@AlexB52, you asked over in the issue:

It seems that rails/webpacker#1990 will be able to configure listen in a rails initializer too right?

Yes, exactly. I've read through a couple of those issues and they were able to be closed without needing this... so I get a little worried that the feature here that would allow them to be silenced might simply mask a more general issue that still wasn't fixed. But I think that's an ok risk to take.

@ColinDKelley
Copy link
Collaborator Author

Having access to Listen gem directly allows configuration but what about people using Listen indirectly through guard for example. I'm not super familiar with guard, would people need to be able to silence these or would it be the gem using Listen to make a port for Listen configuration.

I'm not super-familiar with guard either. If you're using Listen in the same process, then you can simply configure it there, through the Listen.adapter_warn_behavior = interface.

But if it's a command-line interface, that interface would need to expose this Listen configuration as an option somewhere. One hacky way to do that would be as an environment variable, I suppose. Listen already has a few of those already.

@AlexB52
Copy link
Contributor

AlexB52 commented Feb 22, 2024

But if it's a command-line interface, that interface would need to expose this Listen configuration as an option somewhere. One hacky way to do that would be as an environment variable, I suppose. Listen already has a few of those already.

Makes sense, thanks.

Yes, exactly. I've read through a couple of those issues and they were able to be closed without needing this... so I get a little worried that the feature here that would allow them to be silenced might simply mask a more general issue that still wasn't fixed. But I think that's an ok risk to take.

This is how I currently silence these warnings in retest by monkey patching the SymlinkDetector#_fail which seems not ideal. https://github.com/AlexB52/retest/blob/main/lib/config/listen.rb.
Looking at it I can't remember why I used silence_warnings method with a block AND override #_fail.

I'll definitely try to migrate over the :log or :slient adapter.
I was concerned about ruby support. Retest currently supports Ruby 2.5, and uses Listen 3.2. I see that Listen supports Ruby 2.4 which is nice and should be straight forward.

Copy link

@houndci-bot houndci-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: RuboCop found unknown Ruby version 3.0 in `TargetRubyVersion` paramete...
Error: RuboCop found unknown Ruby version 3.0 in `TargetRubyVersion` parameter (in .rubocop.yml).
Supported versions: 2.4, 2.5, 2.6, 2.7, 2.8

@ColinDKelley
Copy link
Collaborator Author

@AlexB52 For separate processes like guard, what do you think of this to support an environment variable LISTEN_GEM_ADAPTER_WARN_BEHAVIOR =? That doesn't have the power of a callable object that can look at the message, but it covers the static cases of warn, log and silent.

Do you think that's sufficient for now?

Invoca@cb8c9c5

@AlexB52
Copy link
Contributor

AlexB52 commented Feb 23, 2024

@ColinDKelley

The environment variable is a nice addition and the change provides plenty of options for people to silence logger warnings.

That said I see a few issues unless I'm misunderstanding something.

TL;DR

  1. The Kernel warnings are still not silenced or piped to the logs.
  2. Logger warnings can now be piped to STDERR with the :warn adapter which wasn't possible before

1. What about Kernel.warn and Kernel#warn?

Unless mistaken, the change still uses Kernel.warn and Kernel#warn on these files which are the ones I'd like to mute when I raised #572. Looking at my previous PR #574, I would expect more spec and lib files to change. Example: symlink_detector is the warning I would love to get rid of nicely. There are so many of them.

It looks like the change still needs to replace Kernel.warn and warn by Logger.adapter_warn across these 5 files.

2. More warnings piped to STDERR

In case we set the adapter to :warn, error messages from Logger.warn will now be printed to STDERR (which weren't before). Is that a regression issue?

If we're using Logger.adapter_warn(message) instead of Kernel.warn then there are no regression issues with existing Logger.warn messages.

Copy link

@houndci-bot houndci-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: RuboCop found unknown Ruby version 3.0 in `TargetRubyVersion` paramete...
Error: RuboCop found unknown Ruby version 3.0 in `TargetRubyVersion` parameter (in .rubocop.yml).
Supported versions: 2.4, 2.5, 2.6, 2.7, 2.8

@ColinDKelley ColinDKelley force-pushed the issue#572/make-kernel-warn-configurable branch from e688838 to 6197c92 Compare February 23, 2024 21:43
Copy link

@houndci-bot houndci-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: RuboCop found unknown Ruby version 3.0 in `TargetRubyVersion` paramete...
Error: RuboCop found unknown Ruby version 3.0 in `TargetRubyVersion` parameter (in .rubocop.yml).
Supported versions: 2.4, 2.5, 2.6, 2.7, 2.8

@ColinDKelley
Copy link
Collaborator Author

@AlexB52 I just pushed the 5 changes to call adapter_warn with specs here.

Notice that I left a SymlinkDetector#warn method--with a deprecation comment--since you mentioned having monkey patched that as a work-around. By stopping off there, I think we can avoid breaking any code like that.

@AlexB52
Copy link
Contributor

AlexB52 commented Feb 23, 2024

@ColinDKelley

Sounds good. It's a good point that people could have monkey-patched the #warn method.
I tested locally with your branch on a local repository that I know shows the symlink warnings

configuration expectation result
LISTEN_GEM_ADAPTER_WARN_BEHAVIOR=warn shows ok
LISTEN_GEM_ADAPTER_WARN_BEHAVIOR=silent muted ok
LISTEN_GEM_ADAPTER_WARN_BEHAVIOR=log muted ok
Listen.adapter_warn_behavior = ->(message) { :warn } shows ok
Listen.adapter_warn_behavior = ->(message) { :silent } muted ok
Listen.adapter_warn_behavior = ->(message) { :log } muted ok
Listen.adapter_warn_behavior = :warn shows ok
Listen.adapter_warn_behavior = :silent muted ok
Listen.adapter_warn_behavior = :log muted ok

Note: I did not try to test whether the warnings were pushed to a log file but only that logs were muted with :log configuration.

@ColinDKelley
Copy link
Collaborator Author

That looks great, @AlexB52. Thanks for the thorough testing.

I'm good with merging it now. Sound good?

@AlexB52
Copy link
Contributor

AlexB52 commented Feb 24, 2024

@ColinDKelley yup sounds good

@ColinDKelley ColinDKelley merged commit 7b7a2f5 into guard:master Feb 24, 2024
15 checks passed
@ColinDKelley ColinDKelley deleted the issue#572/make-kernel-warn-configurable branch February 24, 2024 20:24
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.

3 participants