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

Use hasfield to check whether logger.stream is legal #37

Merged
merged 3 commits into from
Feb 17, 2022

Conversation

fonsp
Copy link
Contributor

@fonsp fonsp commented Oct 3, 2020

An AbstractLogger is not guaranteed to have this field defined.

This fixes an issue where setting a custom logger as global logger will break Suppressor.jl

@fonsp
Copy link
Contributor Author

fonsp commented Nov 10, 2020

🥞

@Krastanov
Copy link

What is the status of this pull request? I am encountering this error when trying to use LoggingExtras JuliaLogging/LoggingExtras.jl#47

@Krastanov
Copy link

@omus Sorry to bother you with this, but you are the only committer I see is part of JuliaIO. Any chance you can given an ETA on this pull request (from last October) or maybe directing me to the appropriate developer whom I should ping?

@tfiers
Copy link

tfiers commented Feb 9, 2022

For others discovering this PR: until it is merged, you can install @fonsp's patched version like so:

  • pkg> add https://github.com/fonsp/Suppressor.jl.git#patch-1,
  • or programmatically (e.g. in atreplinit in startup.jl):
    Pkg.add(url="https://github.com/fonsp/Suppressor.jl.git", rev="patch-1").

@omus
Copy link
Member

omus commented Feb 17, 2022

@Krastanov I apologize for missing your ping. I'll also make sure I watch this package as I didn't realize I was one of the only maintainers :S

@Krastanov
Copy link

Thanks, @omus ! Let me know if I can help (obviously maintainer status requires some vetting, but I can try to help, especially given how "pretty much complete" this project is)

@@ -25,7 +25,7 @@ macro suppress(block)
# approach adapted from https://github.com/JuliaLang/IJulia.jl/pull/667/files
logstate = Base.CoreLogging._global_logstate
logger = logstate.logger
if logger.stream == original_stderr
if :stream in propertynames(logger) && logger.stream == original_stderr
Copy link
Member

@omus omus Feb 17, 2022

Choose a reason for hiding this comment

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

Just for reference normally I'd either use Compat.jl so you can use hasproperty or do:

if !isdefined(Base, :hasproperty)
    hasproperty(x, s::Symbol) = s in propertynames(x)
end

so you can use the nicer syntax. I'll forgo making change though as people have waited long enough for this.

@omus
Copy link
Member

omus commented Feb 17, 2022

Just noticed this package is still using Travis for CI. I'll merge this but address the CI situation before making a new release

@omus omus merged commit f0ee45c into JuliaIO:master Feb 17, 2022
@omus
Copy link
Member

omus commented Feb 18, 2022

Fix has been registered in version 0.2.1

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