Skip to content

Change warnings to Errors in Date extension #4976

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

Closed
wants to merge 1 commit into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Dec 7, 2019

No description provided.

@Girgias Girgias force-pushed the datetime-warnings-to-error branch from 79e2174 to a2fc26e Compare December 7, 2019 02:24
@Girgias Girgias force-pushed the datetime-warnings-to-error branch from a2fc26e to e3ffc8f Compare December 7, 2019 03:08
@cmb69 cmb69 requested a review from derickr December 7, 2019 08:33
@derickr
Copy link
Member

derickr commented Dec 9, 2019

I'm not so keen, as this is a BC break.

@Girgias
Copy link
Member Author

Girgias commented Dec 9, 2019

I'm not so keen, as this is a BC break.

Are you opposed to the change in general or to some specific instances?

Because some of them do fall in line with the various other changes I've made to throw on programming/logic errors.

@cmb69
Copy link
Member

cmb69 commented May 12, 2020

In my opinion, some of these changes are fine (e.g. throwing on "Timezone database is corrupt"), but some appear to be too aggressive (e.g. throwing on "Failed to parse interval"). Basically, it boils down to whether the programmer has a chance to validate user-supplied input, or whether it would be necessary to try calling the function/method.

@Girgias
Copy link
Member Author

Girgias commented May 12, 2020

In my opinion, some of these changes are fine (e.g. throwing on "Timezone database is corrupt"), but some appear to be too aggressive (e.g. throwing on "Failed to parse interval"). Basically, it boils down to whether the programmer has a chance to validate user-supplied input, or whether it would be necessary to try calling the function/method.

Which seems totally reasonable, I'll need to rebase and rework this anyway when I've got time to use the new argument version of the Value errors.

@Girgias
Copy link
Member Author

Girgias commented May 22, 2020

Superseded by #5613

@Girgias Girgias closed this May 22, 2020
@Girgias Girgias deleted the datetime-warnings-to-error branch May 22, 2020 15:11
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