Catch exception around much smaller piece of code
State: closed

Broken off of #23675, followed by refactor.

The refactor has a non-trivial diff, but the only actual _logic_ changed here is the `except ValueError as e`, instead of catching everything from L240-L303, now specifically only catches errors raised by `tslib.array_to_datetime`.  Everything else is just moving code outside of the try/except block and de-indenting.

Catching more specific exceptions is worthwhile in its own right, but this is also an important step for implementing `DatetimeArray._from_sequence` (@TomAugspurger, @jorisvandenbossche, @jreback) since after this we can isolate the part of `to_datetime` called by `DatetimeIndex.__new__` (xref #23675) to make the constructor non-circular (and then refactor it out into `_from_sequence`).

cc @mroeschke 

Labels:
name: Refactor
description: Internal refactoring of code
name: Datetime
description: Datetime data dtype

Comments:
Hello @jbrockmendel! Thanks for updating the PR.

 - In the file [`pandas/core/tools/datetimes.py`](https://github.com/jbrockmendel/pandas/blob/e725875f42dc45a025d4659786300203155ecb18/pandas/core/tools/datetimes.py), following                are the PEP8 issues :
> [Line 250:12](https://github.com/jbrockmendel/pandas/blob/e725875f42dc45a025d4659786300203155ecb18/pandas/core/tools/datetimes.py#L250): [E111](https://duckduckgo.com/?q=pep8%20E111) indentation is not a multiple of four


 - There are no PEP8 issues in the file [`pandas/tests/indexes/datetimes/test_tools.py`](https://github.com/jbrockmendel/pandas/blob/e725875f42dc45a025d4659786300203155ecb18/pandas/tests/indexes/datetimes/test_tools.py) !



##### Comment last updated on November 15, 2018 at 02:51 Hours UTC
Azure fail looks unrelated
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23702?src=pr&el=h1) Report
> Merging [#23702](https://codecov.io/gh/pandas-dev/pandas/pull/23702?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/b4b945adad587851d570e950d05a1da75c5dc76c?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `87.5%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/23702/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/23702?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #23702      +/-   ##
==========================================
+ Coverage   92.24%   92.25%   +<.01%     
==========================================
  Files         161      161              
  Lines       51339    51343       +4     
==========================================
+ Hits        47360    47364       +4     
  Misses       3979     3979
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.64% <87.5%> (ø)` | :arrow_up: |
| #single | `42.34% <50%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23702?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/tools/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23702/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvdG9vbHMvZGF0ZXRpbWVzLnB5) | `85.4% <87.5%> (+0.13%)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/23702/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.02% <0%> (ø)` | :arrow_up: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/23702/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `93.68% <0%> (ø)` | :arrow_up: |
| [pandas/core/groupby/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/23702/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGJ5LnB5) | `96.51% <0%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23702?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23702?src=pr&el=footer). Last update [b4b945a...e725875](https://codecov.io/gh/pandas-dev/pandas/pull/23702?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

The changes seem fine, but I'm not familiar with this section of the code (and in particular I don't know how well tested the expected failure conditions are).
Looks like azure failure in is in Hypothesis
> The changes seem fine, but I'm not familiar with this section of the code (and in particular I don't know how well tested the expected failure conditions are).

AFAIK the tests specific to this file are mostly in tests.indexes.datetimes.test_tools.  I'll need to take a closer look at these tests to see if they can be made more specific, but so far they seem fairly thorough
Is there any behavioural difference?
> Is there any behavioural difference?

Dangit, I just found a corner case in which there is:

```
>>> ts = pd.Timestamp.now()
>>> pd.to_datetime([ts], format='%Y%m%d')
```

returns DTI in master, raises `ValueError` in PR.  Of course the user is a monster for passing this input, but still.

Another round of edits coming up in a bit.
Hmm, that seems like something that does not necessarily need to work .. (if a user passes format, shouldn't the value be strings?) 

Back to my original question: there is no *intended* behavioural change?
> there is no intended behavioural change?

Correct.
But what was then the original reason to do this? There is some change I would assume? (didn't follow the discussion in the PR you are referencing) I now only catch more specific error. But so you encountered a case where another error was catched that should not have been catched?
> But what was then the original reason to do this? 

Following this refactor, the call to `array_to_datetime`, along with the try/except that wraps it, can be refactored into a more tightly-scoped function _doesnt_ call `DatetimeIndex.__new__` or `DatetimeIndex._simple_new`.  This function could then be called from `DatetimeIndex.__new__` (per discussion in #23675) instead of `to_datetime`, resolving the existing circularity.

(Good call asking me to make that explicit; I should have explained that from the start)
@mroeschke you recently implemented the timezone checking in tslib.array_to_datetime for strings there.  Could the same machinery be extended to handle datetimes?  Because AFAICT that's the only thing that conversion.datetime_to_datetime64 does.  If we can get that logic in one place, we can avoid the fallback to datetime_to_datetime64 (which is itself only called in two places, so we may be able to remove it entirely)
@jbrockmendel Definitely. Not sure what it would look like since I had to weave it into the beast of `tslib.array_to_datetime`, but I don't see why not. 
Good news everyone!  I found a case that behaves differently on OSX vs Linux in master

```
now = Timestamp.now()
values = np.array([now, "2018-11-12"], dtype=np.object_)
result = to_datetime(values, format="%Y%m%d",
                     infer_datetime_format=True)
```

raises on Linux, returns correct result on OSX.  Both cases in py27.  (waiting for setup.py build_ext to run so i can check 36/37)

This is turning out hairier than expected.  I think The Right Way to do this involves combining array_to_datetime with datetime_to_datetime64 as discussed above, and probably making the behavior in the `require_iso8601` case more strict (but I'll open a separate issue and/or PR for that)
In py3 this raises on both OSX and Linux
I'm... not happy with how this is turning out.  Closing.