-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat: replaced mathematical kwargs in scheduler constructors #60
Conversation
Good to go? |
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.
I'm happy with your chosen naming scheme, and most of the changes here are broadly good to go.
I would prefer to release a patch with deprecations first. Can we do something like this:
function depkwargs(fn::Symbol, kwargs, remaps::Pair...)
remaps = Dict(remaps...)
kwargs = map(keys(kwargs)) do kw
if haskey(remaps, kw)
Base.depwarn("Keyword $kw is deprecated. Replacing with $(remaps[kw]) instead.", fn)
return remaps[kw] => kwargs[kw]
else
return kw => kwargs[kw]
end
end
return (; kwargs...)
end
function Triangle(; kwargs...)
kwargs = depkwargs(:Triangle, kwargs, :λ0 => :l0, :λ1 => :l1)
return Triangle(kwargs.l0, kwargs.l1, kwargs.period)
end
Very clever! Added to all constructors modified in this PR. Thanks for the snippet! |
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.
One more small change and then we are good to go. Can you also bump the patch version?
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 good, thank you! The nightly failure is unrelated to the PR.
I'm still trying to figure out what happened here, as it turns out, my force push after a The body of |
Closes #41.
For cyclic schedulers, I've replaced
λ*
withl*
, but I'm open to suggestions. At this moment I preferred replacing only the symbols instead of making more drastic changes to the API. Tests and docs were also adapted!cc.: @darsnack