API/COMPAT: add pydatetime-style positional args to Timestamp constructor
State: closed

- [X] closes #10758
- [X] tests added / passed
- [X] passes `git diff upstream/master | flake8 --diff`
- [x] whatsnew entry

If more tests are desired, please let me know where to put them. Same for the whatsnew entry. This is my first pull request for Pandas.

cc @jreback 


Labels:
name: Datetime
description: Datetime data dtype
name: API Design

Comments:
pls add a whatsnew entry.

any luck with accepting the kwargs as well? (or too complicated)

Should the whatsnew go in for 18.1 or something else?

kwargs got too complicated. I wanted to reach a working version of positional parameters first before building kwargs on top of it. I will try out an idea this weekend.

you can put in 0.18.1; if you finish up before we release we can just move

@thejohnfreeman can you rebase / update & move whatsnew to 0.18.2

Ok, I rebased. I have time this week to finish this patch. I wrote a longer comment asking a question, but I'll just go ahead and try the signature you gave above.

```
def __new__(cls,
        object ts_input, object offset=None, tz=None, unit=None,
        year=None, month=None, day=None,
        hour=None, minute=None, second=None, tzinfo=None):
```

## [Current coverage](https://codecov.io/gh/pydata/pandas/pull/12482?src=pr) is **84.12%**

> Merging [#12482](https://codecov.io/gh/pydata/pandas/pull/12482?src=pr) into [master](https://codecov.io/gh/pydata/pandas/branch/master?src=pr) will not change coverage

``` diff
@@             master     #12482   diff @@
==========================================
  Files           138        138          
  Lines         50384      50384          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          42382      42382          
  Misses         8002       8002          
  Partials          0          0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last updated by [82f54bd...79d63d4](https://codecov.io/gh/pydata/pandas/compare/82f54bd1dd53cb031e5d801405b34f062155d823...79d63d4226359303751112ec26281c316596bfc7)

@thejohnfreeman You will also need to update the Timestamp docstring

also I believe its trivial to close #11630 (or it might already be done). Add a test and should be straightforward.

Take a look at my attempt at supporting both positional and keyword arguments. I removed some tests for TypeError; it just means we are more permissive. I'll fix up the doc string and add the other requested tests tomorrow.

whoosh @thejohnfreeman nice constuctor now! yeah prob need some more tests.

Effectively, we are doing type dependent multiple dispatch here with the `Timestamp` constructor. To be honest, I'm not a big fan of adding multiple paths like this that do such different things with the arguments.

@shoyer that doesn't make any sense. `Timestamp` is a sub-class of `datetime`. It doesn't take the correct (positional, nor keyword args ATM). I don't see the problem here. Yes it also does a lot more, but that's exactly the point, it IS user friendly w/o be ambiguous.

I updated the docstring and added tests. I'm considering this a first draft submitted for approval. If it's approved, I'll squash the commits and we can wait for Travis.

did you add a test for #11630 as well (I _think_ you covered it in code, just need the test)

For #11630, I'm considering the test in test_constructor_positional to be equivalent:

```
ts = Timestamp(2000, 1, 2)
```

I can add a test with the exact parameters from #11630 too. Regardless, I will add the issue in a comment.

@thejohnfreeman yes add that exact test (e.g. 

```
self.assertEqual(repr(Timestamp(2015, 11, 12)),  repr(Timestamp('20151112')))
```

should be enough

> @shoyer that doesn't make any sense. Timestamp is a sub-class of datetime. It doesn't take the correct (positional, nor keyword args ATM). I don't see the problem here. Yes it also does a lot more, but that's exactly the point, it IS user friendly w/o be ambiguous.

You cannot add alternate signatures without adding ambiguity. For example, suppose a naive user tries to pass three string arguments instead of integers into the new constructor. This is invalid for both datetime and the existing Timestamp signature, but because these arguments are supplied as strings, it would still trigger the existing codepath.

Let's compare what they would see:

```
>>> pd.Timestamp('2000', '01', '01')
UnknownTimeZoneError: '01'

>>> datetime.datetime('2000', '01', '01')
TypeError: an integer is required
```

The error from datetime is _much_ more informative.

The standard library `datetime.datetime` class does not accept a string to convert to avoid exactly these sorts of issues. Instead, you are forced to use alternate constructors such as `strptime`.

It is unambiguously poor API design to make functions that do two very different things depending on the exact form of the input.

I'm all for making `pd.Timestamp` more consistent with `datetime.datetime` if you're willing to deal with the hassle of a long deprecation cycle, but the right approach is not to overload the constructor to do something totally different. The right approach would be to switch the constructor entirely.

Personally, I can sympathize with @shoyer. In all languages, I prefer that constructors just directly initialize fields and leave any magic to ordinary functions. Especially in Python, you only get one constructor but you can make unlimited functions for every style of construction you want, and it becomes easier to create custom test objects.

That said, I think it's more important to fit into a project's existing style and API, and I can see an argument for fitting this functionality into the `Timestamp` constructor. For now, I'm going to follow Jeff's instructions on this since that's what I've been doing all along.

What about deprecating passing a single object (string or datetime) to `pd.Timestamp` in favor of using `pd.to_datetime`? That way we could get back to only one constructor eventually.

It would not be the end of the world to add this alternate constructor but IMO it would not be a positive improvement to have both.

If to do do it, let's clearly document the two constructors at the top of the docstring for the class, e.g.,

```
Timestamp(ts_input, object_offset=None, tz=None, unit=None)
Timestamp(year, month, day[, hour[, minute[, second[, microsecond[, tzinfo]]]]])

Timestamp is...
```

I _think_ sphinx can process that correctly but we should double check.

@shoyer why would you blow up a very useful interface?

`Timestamp('20130101')` is such a useful feature and you want to DEPRECATE?

the entire point is to REDUCE mental effort here. The whole notion of a user having to choose alternate constructors (and I know we had this discussion w.r.t. `RangeIndex`) is completely bonkers to me.

Sure if things are ambiguous, then you have to choose. In this case it is wholly unambiguous, replicates the existing (python `datetime.datetime` API), and provides very useful parsing abilities, not to mention its very light weight.

> Timestamp('20130101') is such a useful feature and you want to DEPRECATE?

I agree -- my proposal is not entirely serious. I do think we need to make hard choices like this to achieve reliable APIs.

> the entire point is to REDUCE mental effort here. The whole notion of a user having to choose alternate constructors (and I know we had this discussion w.r.t. RangeIndex) is completely bonkers to me.

Just to prove I'm not crazy, here are some high lights from the first page of a Google search for "Python multiple constructors":
http://stackoverflow.com/questions/2164258/multiple-constructors-in-python
http://stackoverflow.com/questions/682504/what-is-a-clean-pythonic-way-to-have-multiple-constructors-in-python
http://www.erol.si/2016/01/python-and-multiple-constructors/
https://pythonconquerstheuniverse.wordpress.com/2010/03/17/multiple-constructors-in-a-python-class/ (read the comments)

I could go on, but you get the point.

You'll notice that the universal advice seems to be to skip the magic and use classmethods for alternate constructors instead of overloading `__init__`.

If we are to add more code paths in the constructor, is it possible to make the entry points more obvious by calling specific `from_*` methods inside `__new__` or `__init__`?  After reading the doc string I feel like it might become harder for a casual contributor like me to debug the code.  But I think this can be remedied by making the intention of each code path more visible in the code.

Hi @kawochen, I too want to make the code paths understandable. I had added some comments to each new path to document its intention. Did they help clarify at all?

I keep getting lint errors in the automated check. It doesn't say what the error is, and running trace8 locally doesn't give me any indication either.

```
The command "ci/script.sh" exited with 1.
1.00s$ ci/lint.sh
inside ci/lint.sh
discarding /Users/travis/miniconda/envs/pandas/bin from PATH
prepending /Users/travis/miniconda/envs/pandas/bin to PATH
NOT Linting
```

Any ideas what I need to do to get Travis to pass?

it's not lint errors but a failed test

https://travis-ci.org/pydata/pandas/jobs/129782361

Ah, it expects `Timestamp(None)` to return Not a Time (`NaT`).

lgtm. @thejohnfreeman nice explanations in the comments. I think it is clear.

@jorisvandenbossche @shoyer 

comments?

@thejohnfreeman did you add the test for #11630 (e.g the `repr` expr from above)

thanks @thejohnfreeman great PR!

Thank you!