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

New API: ProgressLogging.implementedby(logger) #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tkf
Copy link
Collaborator

@tkf tkf commented Mar 9, 2020

This is useful for progress log record providers to check if users have progress monitors. Ref: SciML/DiffEqBase.jl#450 (comment)

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).

@codecov-io
Copy link

codecov-io commented Mar 9, 2020

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%) ⬆️

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.

to be compatible with asprogress
@tkf tkf changed the title New API: ProgressLogging.implemented_by(logger) New API: ProgressLogging.implementedby(logger) Mar 9, 2020
@c42f
Copy link
Member

c42f commented Mar 9, 2020

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 JuliaLang/julia#33960? (I really should finish that one off...)

@tkf
Copy link
Collaborator Author

tkf commented Mar 9, 2020

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 SciML/DiffEqBase.jl#450 was about.)

@tkf
Copy link
Collaborator Author

tkf commented Mar 9, 2020

we should replace in the longer term by generalizing shouldlog somehow

How about this? JuliaLang/julia#33960 (comment)

@c42f
Copy link
Member

c42f commented Mar 17, 2020

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

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