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

extend Base.contains #39

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

extend Base.contains #39

wants to merge 2 commits into from

Conversation

palday
Copy link
Member

@palday palday commented Jun 14, 2022

extend Base.contains using a precise type signature so that contains can be used without qualification. No need to export something from Base.

palday added 2 commits June 14, 2022 10:26
extend `Base.contains` using a precise type signature so that `contains` can be used without qualification. No need to export something from Base.
@palday palday requested a review from ararslan June 14, 2022 15:35
@ararslan
Copy link
Member

While I'm definitely sympathetic to this (I've wanted it for quite some time), it was purposefully excluded. See #5 (comment).

Note also that as implemented, this constitutes a breaking change; it will no longer Just Work™ for arbitrary arguments that implement start and stop.

@ararslan
Copy link
Member

I wonder if there's a different verb we can use so that have our cake and eat too, so to speak, i.e. we can leave the arguments untyped for fans of duck typing and also export without conflicting with a Base function. Could be worth an issue to discuss.

@omus
Copy link
Member

omus commented Jun 14, 2022

If we want to keep using contains as a verb we can always declare our own function (like we were doing) but also fall back on Base for other cases.

TimeSpans.contains(a::TimeSpan, b::Union{TimeSpan,Period}) = ...
TimeSpans.contains(a, b) = Base.contains(a, b)

This allows users to opt-in to the TimeSpans change but not have to always qualify their usage.

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