-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add functions to check enabled checkpoints #26
Conversation
Codecov Report
@@ Coverage Diff @@
## main #26 +/- ##
==========================================
+ Coverage 94.89% 94.92% +0.03%
==========================================
Files 5 5
Lines 137 138 +1
==========================================
+ Hits 130 131 +1
Misses 7 7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work overall. Some suggestions but feel free to merge when ready.
src/Checkpoints.jl
Outdated
enabled() = filter(is_enabled, available()) | ||
|
||
""" | ||
is_enabled(name) -> Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_enabled(name) -> Bool | |
_isenabled(name) -> Bool |
i.e. no _
to be consistent with julia convention, e.g. isa isqrt isone isodd isnan isinf isdir iszero isreal
.
My preference is usually to keep the API as small as possible, because that reduces the probability of having to make a breaking change. So in this case I would just go with exporting enabled()
. We can always export _isenabled
later if there is a need.
p.s. we should export enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is usually to keep the API as small as possible
That makes sense, I was thinking that we may as well make it "available" (not exported but not _
'd either). But it's not needed yet.
I didn't export enabled
by analogy to available
not being exported. Does that change your view at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like config
is not exported either (I think we should export it).
Let's export enabled
, but not available
(since there seems to be no use case for available
outside of the package, but there is for enabled
).
@oxinabox thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A reason I see for not exporting these functions is readability in user code. It's not obvious that config
is about checkpoints if the call is e.g. config(["value1", "value2"], path)
. Even more so for enabled()
on its own. If we exported it, I would prefer to call it enabled_checkpoints
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mzgubic let me know if it looks good now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
src/Checkpoints.jl
Outdated
Returns `true` if the checkpoint `name`(s) are [`config`](@ref)ured. | ||
""" | ||
is_enabled(name) = haskey(CHECKPOINTS, name) && CHECKPOINTS[name] !== nothing | ||
is_enabled(names...) = all(is_enabled.(names)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_enabled(names...) = all(is_enabled.(names)) |
Do we need that? Also, it is probably confusing to see is_enabled
(i.e. singular) with several arguments.
This PR adds two functions:
is_enabled(name)
: returnstrue
if the checkpointname
has beenconfig
ured.enabled()
: returns a vector of all enabled (config
ured) checkpoints.I think both would be useful for users.