DEPR: _BaseOffset's parameter `n` should be prevented from being 0
State: closed

- [ ] closes #31184 
- [ ] tests added / passed
- [ ] passes `black pandas`
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry


Labels:
name: Frequency
description: DateOffsets
name: Deprecate
description: Functionality to remove in pandas

Comments:
I think @jbrockmendel has done similar work on these in the past
Are we sure this should be enforced for _all_ (non-tick) DateOffset subclasses?  `Week` comes to mind as a possible exception.
@jbrockmendel As I noted on the issue, n=0 is currently docced as a rollforward for the general DateOffset class, although it's suggested that "since 0 is a bit weird, we suggest avoiding its use". Perhaps the first step should be officially deprecating n=0 DateOffsets before removal?

With Week, so far as I can tell Week(n=0) does absolutely nothing (not even a rollforward). Not sure whether that speaks in its favor (doing nothing for a 0 offset is unambiguous and in some sense logical) or against it (using it doesn't actually do anything)?

```python
In [3]: pd.to_datetime('2020-01-22') + pd.offsets.Week(n=0)
Out[3]: Timestamp('2020-01-22 00:00:00')

In [4]: pd.to_datetime('2020-01-22') + pd.offsets.Week(n=1)
Out[4]: Timestamp('2020-01-29 00:00:00')

In [5]: pd.to_datetime('2020-01-21') + pd.offsets.Week(n=0)
Out[5]: Timestamp('2020-01-21 00:00:00')

In [6]: pd.to_datetime('2020-01-21 01:29') + pd.offsets.Week(n=0)
Out[6]: Timestamp('2020-01-21 01:29:00')
```
> although it's suggested that "since 0 is a bit weird, we suggest avoiding its use". Perhaps the first step should be officially deprecating n=0 DateOffsets before removal?

I think this makes sense, cc @jreback @mroeschke 

> With Week, so far as I can tell Week(n=0) does absolutely nothing (not even a rollforward).

`Week(n=whatever, normalize=False, weekday=None)` is equivalent to `Day(n=7*whatever)`.  It's kind of annoying to have that one tick-like non-Tick.  Could deprecate the weekday=None option (I'm getting off topic)



@jbrockmendel Sorry, having some trouble with this one.

I've added a FutureWarning to `_validate_n` from `pandas/_libs/tslibs/offsets.pyx::_BaseOffset`, re-ran
```
$ python setup.py develop
```
and then added the decorator
```
    @pytest.mark.filterwarnings("ignore::FutureWarning")
```
to `_check_offsetfunc_works` from `pandas/tests/tseries/offsets/test_offsets.py::TestCommon`.

However, when I run
```
$ pytest pandas/tests/tseries/offsets/test_offsets.py::TestCommon::test_apply
```
I still get
```
AssertionError: Caused unexpected warning(s): [('FutureWarning', FutureWarning('Deprecated n=0'), '/home/SERILOCAL/m.gorelli/pandas/pandas/tseries/offsets.py', 1290)]
```

(note: I'm aware that "Deprecated n=0" isn't an adequate warning message, I just wanted to try getting this working before looking into how to write the message properly)
> @pytest.mark.filterwarnings("ignore::FutureWarning")

Instead, use `with tm.assert_produces_warning(FutureWarning):`
@jbrockmendel sorry to tag you here again, just don't know what to make of the CI failure. If I run 
```
$ python setup.py develop
$ pytest pandas/tests/arithmetic/test_numeric.py
```
locally on Linux, Python 3.7.6, it passes. But during the "Linux py37_np_dev" CI job, this test fails.

Any suggestions on why this may be would be appreciated, else I'll keep trying to figure it out :)

> Instead, use with tm.assert_produces_warning(FutureWarning):

Just a note on this: the part of the code where I wanted to apply this (pandas/tests/tseries/offsets/test_offsets.py, line 367) doesn't always produce a ``FutureWarning``, so I wanted to just ignore the warning. Have used ``warnings.filterwarnings``

EDIT
----

Found it, I need to run the tests with the `-W error` flag
It looks like the n!=0 restriction is being applied to offsets.Second, when it shouldn't apply to any of the Tick offsets.
@jbrockmendel - thanks, seems this is passing now. The [travis job](https://travis-ci.org/pandas-dev/pandas/builds/647315576?utm_source=github_status&utm_medium=notification) says it's passed if I click on it but still shows as pending here
> this is removing an awful lot of tests. can you comment on some of the removals why we no longer test these

@jreback Some of these, e.g.
```
    apply_cases.append(
        (
            BDay(0),
            {
                datetime(2008, 1, 1): datetime(2008, 1, 1),
                datetime(2008, 1, 4): datetime(2008, 1, 4),
                datetime(2008, 1, 5): datetime(2008, 1, 7),
                datetime(2008, 1, 6): datetime(2008, 1, 7),
                datetime(2008, 1, 7): datetime(2008, 1, 7),
            },
        )
    )
```
cause a warning error during test collection (because of `BDay(0)`). I can catch the warning / rewrite the expected value if this happens in the middle of a test, but don't know of a way to do that during test collection.
side-note: we should look into restricting FY5253 startingMOnth/weekday to be non-None