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

Add missing support for addition and subtraction #37222

Merged
merged 6 commits into from
Aug 31, 2020

Conversation

nilshg
Copy link
Sponsor Contributor

@nilshg nilshg commented Aug 26, 2020

Inspired by the discussion in #28570.

Tests pass locally with this change.

There are no additional tests for missing support in this PR, it seems to me the methods are so simple that I'm not sure what would be tested here (although I have a feeling saying things like this is why I'm not a software developer...)

@pdeffebach
Copy link
Contributor

You need <, >, <=, and >= as well, right?

Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@nilshg
Copy link
Sponsor Contributor Author

nilshg commented Aug 26, 2020

You need <, >, <=, and >= as well, right?

Those are covered in missings.jl in Base as per discussion on Slack, right?

@pdeffebach
Copy link
Contributor

Yes, no need to add them.

nilshg and others added 3 commits August 26, 2020 20:59
These test the exported subtypes of `AbstractTime`
Avoids the usage of `subtypes` function, which would need to be imported from `InteractiveUtils` otherwise
@nalimilan nalimilan linked an issue Aug 27, 2020 that may be closed by this pull request
Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me.

As discussed at #28570, I think this is justified as all types that support + and - should propagate missing. This is different from adding new missing-propagating functions, which I agree is problematic as it's not clear where to stop. Here the limit is very clear: add methods to all functions that already propagate missing values to ensure consistency.

Fun fact: this missing method was also spotted by pandas developers, who have recently worked on improving their missing values handling and took Julia as a reference.

stdlib/Dates/test/arithmetic.jl Outdated Show resolved Hide resolved
@nalimilan nalimilan added domain:dates Dates, times, and the Dates stdlib module domain:missing data Base.missing and related functionality stdlib Julia's standard library labels Aug 27, 2020
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@nilshg
Copy link
Sponsor Contributor Author

nilshg commented Aug 27, 2020

Thanks, looks good to me.

Thanks for your help!

@quinnj quinnj merged commit 2a96176 into JuliaLang:master Aug 31, 2020
@nilshg nilshg deleted the patch-1 branch September 2, 2020 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dates Dates, times, and the Dates stdlib module domain:missing data Base.missing and related functionality stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date and DateTime support for Missing is missing
4 participants