Conversation
|
@lsegal Apologies for the ping. Would you have a chance to review this PR? Homebrew currently depends on this change for Ruby 4.0 support, and we're pinned to the PR author's fork (one of our lead maintainers) unless this is merged. We'd love to get this upstream. |
| DOUT = Notifier::def_notifier("SLex::") | ||
| D_WARN = DOUT::def_notifier(1, "Warn: ") | ||
| D_DEBUG = DOUT::def_notifier(2, "Debug: ") | ||
| D_DETAIL = DOUT::def_notifier(4, "Detail: ") | ||
|
|
||
| DOUT.level = Notifier::D_NOMSG |
There was a problem hiding this comment.
This will almost certainly break downstream users of IRB on Ruby 2.x/3.x if we remove a feature that is advertised by IRB. It seems unlikely, but I would imagine the cleaner approach would be to only disable this behavior if the require fails.
The other approach would be to vendor notifier or otherwise create a mock impl of Notifier.
There was a problem hiding this comment.
It'll only break (or well "break" in terms of missing debug logging) if they actively required this file within the IRB session and set this private API to enable logging. But couldn't we not just rename the module then? The file is always imported via require_relative for use here so it doesn't need to be called IRB::SLex if we're never using upstream IRB::SLex.
SLex itself was never public API. It was removed in a minor release rather than a major one and went through refactors previously without any backward compatibility.
There was a problem hiding this comment.
No break > break.
Yes, we could move the module, that would be a different PR than a no-op refactor IMO. I would accept a refactor that basically vendored SLex as a separate module since we are relying on it. In that module we could do whatever we wanted, including ... not using the SLex code at all.
3 participants
Description
irb/notifierusage appears to just be debug logs that are never actually activated since they aren't linked up to YARD's own logging system.The dependency on it therefore seems unnecessary and looks safe to remove, fixing compatibility with Ruby 4.0.
Normally this would only be a problem with the legacy parser but overload tag parsing still unconditionally uses the legacy parser only:
yard/lib/yard/tags/overload_tag.rb
Lines 58 to 60 in eddd10c
Fixes #1636
Fixes #1645
Completed Tasks
bundle exec rakelocally (if code is attached to PR).