ProgressLogging.jl icon indicating copy to clipboard operation
ProgressLogging.jl copied to clipboard

New API: ProgressLogging.implementedby(logger)

Open tkf opened this issue 5 years ago • 5 comments

This is useful for progress log record providers to check if users have progress monitors. Ref: https://github.com/JuliaDiffEq/DiffEqBase.jl/pull/450#issuecomment-596315679

Originally I called it enabled_for. But I think implementedby makes more sense. For example, the user may turn off progress logging capability for a particular instance of a custom logger type. In this case, the name enabled_for implies that this function should return false. But if this function is used by log record providers, they may take action incompatible to the user's intent (e.g., warning message telling the user that the current logger does not support progress bar).

tkf avatar Mar 09 '20 04:03 tkf

Codecov Report

Merging #24 into master will increase coverage by 0.17%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
+ Coverage   71.77%   71.95%   +0.17%     
==========================================
  Files           1        1              
  Lines         163      164       +1     
==========================================
+ Hits          117      118       +1     
  Misses         46       46
Impacted Files Coverage Δ
src/ProgressLogging.jl 71.95% <100%> (+0.17%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b64333d...f82af55. Read the comment docs.

codecov-io avatar Mar 09 '20 04:03 codecov-io

This looks reasonable to me if it solves the DiffEqBase issue.

As I said over at DiffEqBase, I feel like it's something we should replace in the longer term by generalizing shouldlog somehow. Maybe somehow in combination with a progress logging category for the level, as in https://github.com/JuliaLang/julia/pull/33960? (I really should finish that one off...)

c42f avatar Mar 09 '20 04:03 c42f

Thanks for having a look at it!

> I feel like it's something we should replace in the longer term by generalizing `shouldlog` somehow.

Wouldn't it be a very long term project? I think it would require parametrized Task type so that current_logger() can be inferred?

(Edit: Never mind. This was not what JuliaDiffEq/DiffEqBase.jl#450 was about.)

tkf avatar Mar 09 '20 05:03 tkf

we should replace in the longer term by generalizing shouldlog somehow

How about this? https://github.com/JuliaLang/julia/pull/33960#issuecomment-596344803

tkf avatar Mar 09 '20 05:03 tkf

Good suggestion, I think something like that is the right way to go.

c42f avatar Mar 17 '20 02:03 c42f