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

Remove null from signature of formatDate #2724

Merged
merged 1 commit into from
Jul 24, 2018

Conversation

ntrinquier
Copy link
Contributor

@ntrinquier ntrinquier commented Jul 24, 2018

Fixes #2612

Changes proposed in this pull request:

As per #2612 - Remove null from the signature of the formatDate function.

Reviewers should focus on:

This is a non-backward compatible API change.

Screenshot

N/A

@giladgray
Copy link
Contributor

I am not convinced by the issue. Gonna write a test real quick.

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

ok seems legit. the code does in fact capture null values so formatDate never receives them.

i feel like there's a use case for formatting the null value but since it's not implemented that way, let's merge this to correct the typings.

also this is definitely backwards-compatible as a (date: Date | null) => string function can be passed to a (date: Date) => string prop.

@giladgray giladgray merged commit 6f1ed47 into palantir:4.x Jul 24, 2018
@giladgray
Copy link
Contributor

@ntrinquier ok wait i just realized you targeted the 4.x branch which is not a thing. please try again on develop.

@giladgray
Copy link
Contributor

sorry, ignore that comment. i can just merge 4.x into develop.

@giladgray
Copy link
Contributor

you definitely should have called that out in the description.

@giladgray
Copy link
Contributor

also for the record, if this were breaking i would have rejected it as there's no way i'm about to start on 4.0.0 😂

thanks for the contribution @ntrinquier!!

@ntrinquier
Copy link
Contributor Author

@giladgray As you said there's likely a use case for formatting the null value, but right now it's confusing since it is never going to be called anyway :)
Thanks for taking care of all the branching!

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.

2 participants