Error handling in break_time

[swapsha96]

Following code returns ValueError and is not handled properly:
from sunpy.time import break_time
break_time('garbage')

Following result is obtained on the terminal:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "sunpy/time/time.py", line 283, in break_time
    return parse_time(t, time_format).strftime("%Y%m%d_%H%M%S")
  File "sunpy/time/time.py", line 199, in parse_time
    raise ValueError("{tstr!s} is not a valid time string!".format(tstr=time_string))
ValueError: garbage is not a valid time string!

This PR uses try-catch to handle the error input in break_time() function. pass is used as it is used in line 186 of same file.

[pep8speaks]

Hello @swapsha96! Thanks for updating the PR.

• In the file sunpy/time/time.py, following are the PEP8 issues :

Line 121:1: E302 expected 2 blank lines, found 1

Comment last updated on April 12, 2017 at 11:33 Hours UTC

[swapsha96]

Thank you @pep8speaks ! I removed unnecessary blank lines. Is it now good to go?

[wafels]

Why is it not OK to have parse_time raise the error?

…

On Wed, Apr 12, 2017 at 7:34 AM, Swapnil Sharma ***@***.***> wrote: Thank you @pep8speaks <https://github.com/pep8speaks> ! I removed unnecessary blank lines. Is it now good to go? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2074 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8CF-l70PykrVQYj2tjutFFcqZ6z3YHks5rvLbjgaJpZM4M7SPY> .

[swapsha96]

@wafels Generally, error handling using try-catch from user side is discouraged. User is supposed handle value returned from a function. Errors like ValueError, NameError, SyntaxError can lead to application crash and thus, function must return appropriate error and user can handle it using if-else condition.

[wafels]

Ok, I get it. So in this case, None is returned? Hmmm. This might be an example of something that could be implemented across sunpy, if we make that decision.

…

On Wed, Apr 12, 2017 at 1:41 PM, Swapnil Sharma ***@***.***> wrote: @wafels <https://github.com/wafels> Generally, error handling using try-catch from user side is discouraged. User is supposed handle value returned from a function. Errors like ValueError, NameError, SyntaxError can lead to application crash and thus, function must return appropriate error and user can handle it using if-else condition. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2074 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8CF7iQWfCcTvFnHquGQkUEbc-aOk-4ks5rvQy-gaJpZM4M7SPY> .

[swapsha96]

@wafels Is this PR alright to be merged? I am ready to find and fix such errors.

[wafels]

I don't know about this PR or the other places in the code where the approach in this PR might be needed. I think this is a coding practices decision that we could choose to enforce across sunpy.

…

On Wed, Apr 12, 2017 at 2:38 PM, Swapnil Sharma ***@***.***> wrote: @wafels <https://github.com/wafels> Is this PR alright to be merged? I am ready to find and fix such errors. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2074 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8CF3jtmigc_0-dR2mBedgFZRA5d6Kpks5rvRoygaJpZM4M7SPY> .

[dpshelio]

I don't agree to fail quietly and return None if the user is not using the function correctly.

[ayshih]

I don't like this PR. Not raising exceptions means that:

• Downstream code can't tell the difference between None as a legitimate return value and None as an error value.
• Downstream code can't tell if a error condition occurred.
• If an error condition is detected by None as a return value, which requires a guarantee that None is not a legitimate return value, downstream code can't tell the difference between different types of error conditions.

Also, returning None is possibly worse than raising an exception because downstream code that doesn't check specifically for None may cause unintended behavior with such return. For example, in this case, imagine someone tried to generate a filename using a None return value. Raising an exception gives direct visibility to the source of the problem.

If this is in fact will be the preferred coding style, presumably we should change parse_time to not raise exceptions in the first place.

[ayshih]

As a side note, can someone remind me why this function is called break_time? It seems like a bizarre, non-intuitive name.