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

Update dependency and fix deprecation #43

Merged
merged 2 commits into from
Sep 5, 2022

Conversation

devmotion
Copy link
Collaborator

I just noticed that TerminalLoggers holds back LeftChildRightSiblingTrees. This PR bumps the dependency and adds a CompatHelper setup to automate such updates in the future.

Moreover, when running the tests a deprecation warning turned up which I fixed. While doing that I came across JuliaLang/julia#29901 and adapted the package accordingly.

@jlperla
Copy link

jlperla commented Aug 22, 2022

Any hope on merging this? I tested @devmotion PR and this worked to fix my turing logging woes after he isolated the issue to this package.

@devmotion Not sure if it matters, but when I did my test it did downgrade the AbstractTrees package. I am not using it directly, but am a little worried since it is a dependency of SymbolicUtils (which supports both 0.3 and 0.4 abstracttrees) which is pretty central to my setup.

The output here was

(HMCExamples) pkg> add https://github.com/devmotion/TerminalLoggers.jl#037403c
   Resolving package versions...
    Updating `C:\Users\jesse\Documents\GitHub\HMCExamples.jl\Project.toml`
  [5d786b92] ~ TerminalLoggers v0.1.0 ⇒ v0.1.5 `https://github.com/devmotion/TerminalLoggers.jl#037403c`
    Updating `C:\Users\jesse\Documents\GitHub\HMCExamples.jl\Manifest.toml`
⌅ [1520ce14] ↓ AbstractTrees v0.4.2 ⇒ v0.3.4
⌅ [1d6d02ad] + LeftChildRightSiblingTrees v0.1.3
  [5d786b92] ~ TerminalLoggers v0.1.0 ⇒ v0.1.5 `https://github.com/devmotion/TerminalLoggers.jl#037403c`
        Info Packages marked with ⌅ have new versions available but cannot be upgraded. To see why use `status --outdated -m`

I tried to manually update those with a ] up LeftChildRightSiblingTrees AbstractTrees but it didn't do anything.
If you don't see anything wrong with this PR then maybe it should be merged and I can try from a free manifest/project file to see how it does?

@devmotion
Copy link
Collaborator Author

It's adding an old version of LeftChildRightSiblingTrees in your setup that is not compatible with AbstractTrees 0.4. You can run ] st --outdated -m in the Julia REPL to see which package in your environment holds it back.

@jlperla
Copy link

jlperla commented Aug 22, 2022

Thanks. The relevant lines seem to be

Status `C:\Users\jesse\Documents\GitHub\HMCExamples.jl\Manifest.toml`
⌅ [1520ce14] AbstractTrees v0.3.4 (<v0.4.2): LeftChildRightSiblingTrees
⌅ [1d6d02ad] LeftChildRightSiblingTrees v0.1.3 (<v0.2.0): TerminalLoggers

So maybe if this TerminalLoggers PR was merged then these things might work themselves out? The accompanying segment in the manifest for the PR is

[[deps.TerminalLoggers]]
deps = ["LeftChildRightSiblingTrees", "Logging", "Markdown", "Printf", "ProgressLogging", "UUIDs"]
git-tree-sha1 = "62846a48a6cd70e63aa29944b8c4ef704360d72f"
repo-rev = "037403c"
repo-url = "https://github.com/devmotion/TerminalLoggers.jl"
uuid = "5d786b92-1e48-4d6f-9151-6b4477ca9bed"
version = "0.1.5"

@devmotion
Copy link
Collaborator Author

This PR does support LCRSTrees 0.2. The problem is that you did not install the PR but the default branch in my fork:

(HMCExamples) pkg> add https://github.com/devmotion/TerminalLoggers.jl#037403c

You should be able to install the PR with e.g. ] add https://github.com/devmotion/TerminalLoggers.jl#dw/compat.

@jlperla
Copy link

jlperla commented Aug 22, 2022

Made the change and I think we are in business. Thanks for your help. Will switch to the official version after this is merged.

@devmotion
Copy link
Collaborator Author

Bump 🙂 @c42f, could you review the PR? Or suggest another reviewer?

Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

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

Looks pretty good, thanks, sorry for the delay!

I feel like we don't need the overload of showvalue here?

Comment on lines 173 to 175
@eval begin
showvalue(io, key, ex::Base.ExceptionStack) = Base.show_exception_stack(io, ex)
end
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary, show() already works for ExceptionStack?

It was intended that we have the small amount of extra decoration of show() vs the internal function show_exception_stack - an exception stack isn't necessarily an error per se. Eg, you can use

@info "we ignored a thing"  current_exceptions()

BTW if this was kept you don't need the @eval here because this is top level code.

@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2022

Codecov Report

Merging #43 (fe8dc71) into master (037403c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #43   +/-   ##
=======================================
  Coverage   95.28%   95.28%           
=======================================
  Files           4        4           
  Lines         318      318           
=======================================
  Hits          303      303           
  Misses         15       15           
Impacted Files Coverage Δ
src/TerminalLogger.jl 94.11% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@c42f
Copy link
Member

c42f commented Sep 5, 2022

I've taken the liberty of removing that small section with showvalue for now so I can trigger tagbot with a new version for TerminalLoggers.jl before I forget! Feel free to open a separate PR for it if you feel strongly about it :-)

@c42f c42f merged commit a05003c into JuliaLogging:master Sep 5, 2022
@c42f
Copy link
Member

c42f commented Sep 5, 2022

Thanks for this, should be released in 0.1.6:

JuliaRegistries/General#67700

@devmotion devmotion deleted the dw/compat branch September 5, 2022 05:03
@devmotion
Copy link
Collaborator Author

Thank you!

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