Lock down kwargs in offsets signatures
State: closed

Explicitly specify allowed kwargs in `__init__` for `Week`, `WeekOfMonth`, `LastWeekOfMonth` and `QuarterOffset`.

Ref #17176 

Intended to be orthogonal/complementary to #17450.

Will add tests after confirming we're on the same page.

- [ ] closes #xxxx
- [ ] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Frequency
description: DateOffsets

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=h1) Report
> Merging [#17458](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/20fee85ede7f2052f855b8f3445cd1ffc17ee0c3?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #17458      +/-   ##
==========================================
- Coverage   91.15%   91.14%   -0.02%     
==========================================
  Files         163      163              
  Lines       49591    49588       -3     
==========================================
- Hits        45207    45195      -12     
- Misses       4384     4393       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.92% <100%> (-0.01%)` | :arrow_down: |
| #single | `40.25% <26.66%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/tseries/offsets.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvb2Zmc2V0cy5weQ==) | `97.14% <100%> (-0.01%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.72% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17458?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/17458?src=pr&el=footer). Last update [20fee85...5e276c9](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=h1) Report
> Merging [#17458](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/81694dce171ecd93a65e32ed455612ee967d3951?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #17458      +/-   ##
==========================================
- Coverage   91.26%   91.24%   -0.03%     
==========================================
  Files         163      163              
  Lines       49872    49978     +106     
==========================================
+ Hits        45514    45600      +86     
- Misses       4358     4378      +20
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.04% <100%> (ø)` | :arrow_up: |
| #single | `40.24% <30.35%> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/tseries/offsets.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvb2Zmc2V0cy5weQ==) | `97.16% <100%> (+0.01%)` | :arrow_up: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/indexing.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhpbmcucHk=) | `93% <0%> (-0.98%)` | :arrow_down: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `94.89% <0%> (-0.16%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.74% <0%> (-0.09%)` | :arrow_down: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `92.03% <0%> (-0.09%)` | :arrow_down: |
| [pandas/core/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tbW9uLnB5) | `91.42% <0%> (ø)` | :arrow_up: |
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `91.76% <0%> (ø)` | :arrow_up: |
| [pandas/core/internals.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzLnB5) | `94.38% <0%> (ø)` | :arrow_up: |
| [pandas/core/indexes/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9wZXJpb2QucHk=) | `92.78% <0%> (+0.01%)` | :arrow_up: |
| ... and [8 more](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/17458?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/17458?src=pr&el=footer). Last update [81694dc...086b485](https://codecov.io/gh/pandas-dev/pandas/pull/17458?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ok, rebase this and i'll look again.
Will rebase, but I haven't gotten around to addressing your earlier comments, so this one isn't ready yet.  #17564, #17585, #17363, #17543, and #17342 should be good to go.
Hello @jbrockmendel! Thanks for updating the PR.





Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on October 06, 2017 at 03:52 Hours UTC
OK, as requested, the new commits cover all of the existing classes.
> looks fine, can you pass thru the timeseries.rst docs and if anything needs changing there (should be ok), but who knows

Didn't see anything that is affected by this PR, but a bunch of things that I want to double-check later given the outstanding GH issues.

pls generate and add at least 1 file from ``generate_legacy_storage_files`` for pickle. I am not convinced this is backward compatible with existing pickles. It might be but can't determine without some tests.
> pls generate and add at least 1 file from generate_legacy_storage_files for pickle. I am not convinced this is backward compatible with existing pickles. It might be but can't determine without some tests.

Following the instructions at the top of `tests.io.test_pickle`, installed 0.16.2, ran (0.20.3's)  "python generate_legacy_storage_files.py ~/Desktop/ pickle", got

```
Traceback (most recent call last):
  File "generate_legacy_storage_files.py", line 282, in <module>
    write_legacy_file()
  File "generate_legacy_storage_files.py", line 274, in write_legacy_file
    write_legacy_pickles(output_dir=output_dir)
  File "generate_legacy_storage_files.py", line 235, in write_legacy_pickles
    pickle.dump(create_pickle_data(), fh, pickle.HIGHEST_PROTOCOL)
  File "generate_legacy_storage_files.py", line 173, in create_pickle_data
    data = create_data()
  File "generate_legacy_storage_files.py", line 155, in create_data
    freq=Timestamp('2011-01-01', freq='D'),
  File "pandas/tslib.pyx", line 274, in pandas.tslib.Timestamp.__new__ (pandas/tslib.c:7571)
TypeError: __new__() got an unexpected keyword argument 'freq'
```

Will try again with 18ish.

Just to double-check: you're aware that test_offsets has `test_pickle_v0_15_2` and `test_pickle_compat_0_14_1`?
Same with 0.18.1
@jbrockmendel you may need to slightly modify the generation script to support back-generation. This is part of the challenge. you can use pandas versions to do that ONLY in that script to do that.  If you make changes then they should be checked in as part of this PR. 

generally we generate the most previous version or 2 with this script. Since you are touching offsets, need to go back and assure pickle compat for those.
Added tests for 0.19.2 and 0.20.3, opened an issue to ask for clarification on these instructions in the docs.
circleCI log looks like the error is:

```
unable to execute 'x86_64-conda_cos6-linux-gnu-gcc': No such file or directory
error: command 'x86_64-conda_cos6-linux-gnu-gcc' failed with exit status 1
```
If you rebase on master that error should go away. That was temporarily fixed in https://github.com/pandas-dev/pandas/pull/17700
see https://github.com/pandas-dev/pandas/pull/17733
@jbrockmendel 
since we really are locking down signatures. I would write an old (but acceptable signature) and add to ``generate_legacy_storage_files`` (e.g. maybe with the kwargs or something). I think this will all work, just want to confirm.
I think I covered this case ``pandas/tests/tseries/data/dateoffset_0_15_2.pickle`` as well, so can remove that (now I didn't run it for 0.15.2 so you can certainly do that).
Thanks for fixing the script.  Added a file for 0.20.3 for kicks.

So since `test_pickle_v0_15_2` isn't an example for what you want a test to look like I'm guessing  iterate over the files unpickling them and checking that the `data['offsets']` matches the `off` dict defined in `generate_legacy_storage_files.create_data`?

> I think I covered this case `pandas/tests/tseries/data/dateoffset_0_15_2.pickle` as well, so can remove that (now I didn't run it for 0.15.2 so you can certainly do that).

There's some subject-verb ambiguity here.  What did you cover/run?  What can be removed?

> I would write an old (but acceptable signature) and add to generate_legacy_storage_files (e.g. maybe with the kwargs or something)

So go into `generate_legacy_storage_files` and add something to the `off` dict and generate a new pickle file?  But wouldn't that invalidate the existing files?

> So go into generate_legacy_storage_files and add something to the off dict and generate a new pickle file? But wouldn't that invalidate the existing files?

no you can just generate a new one. We only compare to things that exist in the pickle AND in the generate script. So as long as you simply 'add' things its ok (e.g. don't rename or delete).

I added an offset section, you can simply add *moar* things to it (or even another version of it that say initializes with keyword args or somesuch).
OK, so I generated a new file by running the script under 0.20.3.  Then... what?  The existing pickle tests in tests.tseries.test_offsets are precedent you've explicitly said not to follow.
Just added a handful of new examples to `generate_legacy_storage_files`, updated the 0.20.3 file, created a 0.19.2 file.  Attempts to run under py3 raised

```
  File "pandas/pandas/tests/io/generate_legacy_storage_files.py", line 40, in <module>
    from pandas import (Series, DataFrame, Panel,
  File ".virtualenvs/pandas_0.19/lib/python3.6/site-packages/pandas/__init__.py", line 85, in <module>
    v = get_versions()
  File ".virtualenvs/pandas_0.19/lib/python3.6/site-packages/pandas/_version.py", line 23, in get_versions
    return json.loads(version_json)
AttributeError: module 'json' has no attribute 'loads'
```
also seems we should update the doc-strings for the offsets now that the signature are ok, xref #8435
>  File "pandas/pandas/tests/io/generate_legacy_storage_files.py", line 40, in <module>
    from pandas import (Series, DataFrame, Panel,
  File ".virtualenvs/pandas_0.19/lib/python3.6/site-packages/pandas/__init__.py", line 85, in <module>
    v = get_versions()
  File ".virtualenvs/pandas_0.19/lib/python3.6/site-packages/pandas/_version.py", line 23, in get_versions
    return json.loads(version_json)
AttributeError: module 'json' has no attribute 'loads'

This is because we have a top-level file called ``json.py`` for backwards compat (was a c-module),  which shadows the python json library, and the version things actually import this. So in this case you need to temporarily edit the ``pandas/_version.py`` file (in the version you are actually using) to avoid use of json (just change it to return a dict). a bit of a hack, but I have found no other way of fixing this (and it doesn't affect the released version, just running it like this). 
ok this looks fine.  Can you update the doc-strings? (also can do in another PR). now that the keywords are there they should be documented. lmk
> Can you update the doc-strings? (also can do in another PR). 

This PR has been exhausting.  I'll put this on the TODO list.
@jbrockmendel hahah, ok thanks1