API: read_csv, to_csv line_terminator keyword inconsistency
State: closed

- [x] closes #9568
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry (pending 1.2)

I followed discussion in #9568. The aim of this PR is to allow both `line_terminator` and `lineterminator` keyword args  to `read_csv` and `to_csv` but only document `line_terminator`. As per @jorisvandenbossche's suggestion we preserve `lineterminator` to stay compatible with `csv` dialects.


Labels:
name: IO CSV
description: read_csv, to_csv
name: API - Consistency
description: Internal Consistency of API/Behavior

Comments:
I am -1 on this change as I don't think adding kwargs to these functions is a good idea; we struggle enough already with how large these signatures are

I don't know if any change here is really worth the churn
> I am -1 on this change as I don't think adding kwargs to these functions is a good idea; we struggle enough already with how large these signatures are
> 
> I don't know if any change here is really worth the churn

@WillAyd @simonjayhawkins  #27544 might be relevant
That change is about _keyword only_ calls, i.e. you can't call items positionally. That is separate from allowing kwargs, which I think we generally have moved away from because it actually can cause more confusion than good for end users
> That change is about _keyword only_ calls, i.e. you can't call items positionally. That is separate from allowing kwargs, which I think we generally have moved away from because it actually can cause more confusion than good for end users

Ok, right. 

xref #34976 added tests to check for incorrect keyword arguments to `read_csv` - this PR breaks those

The options seem to be:
1. live with things as they are - close this and close #9568
2. (maybe) find a better way to have both `line_terminator` and `lineterminator`. I'm not sure there's a way that doesn't cause at least as much confusion as the current solution.
3. replace `lineterminator` with `line_terminator` in `read_csv`
4. replace `line_terminator` with `lineterminator` in `to_csv`

I'm agnostic on which way to go - but happy to implement the changes if we're doing 2/3/4.
> I think we generally have moved away from because it actually can cause more confusion than good for end users

agree with this point in general, however in this case we want to avoid the inconsistency AND allow both 'line_terminator' and 'lineterminator', see https://github.com/pandas-dev/pandas/issues/9568#issuecomment-342613292 and https://github.com/pandas-dev/pandas/issues/9568#issuecomment-342619796

I'm not keen on having both parameters in the signature, so I feel the **kwargs is the lesser of two evils.

If we change something I would agree with the comment to just add both; it would be less code than kwargs and doesn't have any side effects. We can just document it as `alias for XXX` which I think we do for other things
Checking in on this.

It seems like people _do_ agree on having both `line_terminator` and `lineterminator`. The options are:

1. have both arguments in the signature, and document `lineterminator` as alias to `line_terminator` (+1 @WillAyd)
2. use `line_terminator` in signature and allow `line_terminator` as a kwarg. This would mean adding `kwargs` to both `read_csv` and `to_csv` - currently neither method has those (+1 @simonjayhawkins)

Looping in @jreback @jorisvandenbossche since you guys discussed this back in #9568
What about something like

```
def mywrapper(func):
    @wraps(func)
    def new_func(*args, **kwargs):
        if "lineterminator" in kwargs:
            kwargs["line_terminator"] = kwargs.pop("lineterminator")
        return func(*args, **kwargs)
    return new_func

@mywrapper
def read_csv(...)
```

`inspect.signature` shows the original signature unchanged.  Will that make it to the docs correctly? 
> What about something like
> 
> ```
> def mywrapper(func):
>     @wraps(func)
>     def new_func(*args, **kwargs):
>         if "lineterminator" in kwargs:
>             kwargs["line_terminator"] = kwargs.pop("lineterminator")
>         return func(*args, **kwargs)
>     return new_func
> 
> @mywrapper
> def read_csv(...)
> ```
> 
> `inspect.signature` shows the original signature unchanged. Will that make it to the docs correctly?

I'll compile the docs and check!
> What about something like
> 
> ```
> def mywrapper(func):
>     @wraps(func)
>     def new_func(*args, **kwargs):
>         if "lineterminator" in kwargs:
>             kwargs["line_terminator"] = kwargs.pop("lineterminator")
>         return func(*args, **kwargs)
>     return new_func
> 
> @mywrapper
> def read_csv(...)
> ```
> 
> `inspect.signature` shows the original signature unchanged. Will that make it to the docs correctly?

@jbrockmendel this renders fine in the docs. To move forward, should I add this decorator to `pandas.util._decorators`? I'd like to be systematic (we'd be using this at least twice, in `read_csv` and `to_csv`).

This is very cool - thanks for suggesting it!!
Is this just a case for `deprecate_kwarg` or am I missing something?  Is there a desire to keep both alive for some reason?

That only `line_terminator` is being documented is a strong suggestion that `lineterminator` should not be used (and eventually usable).
> Is this just a case for `deprecate_kwarg` or am I missing something? Is there a desire to keep both alive for some reason?

see https://github.com/pandas-dev/pandas/issues/9568#issuecomment-342613292 and https://github.com/pandas-dev/pandas/issues/9568#issuecomment-342619796

so from the issue, the current 'requirement' is to accept both. `deprecate_kwarg` is certainly the easier option.

> so from the issue, the current 'requirement' is to accept both. `deprecate_kwarg` is certainly the easier option.

Deprecating doesn't mean it has to be removed -- it just tells users not to use it. Technically I think `deprecate_kwarg` satisfies the requirements of the old thread since both are accepted and only the preferred is retained.


@bashtage @simonjayhawkins I'm down to do either (implement the decorator in a separate PR then come back here or deprecate)
To be honest I don't think this is that big of an issue that we need to add another layer of wrapping to the csv function, so slight -1 on that design
How about in `read_csv` deprecating `lineterminator` in favour of `line_terminator` and leaving `to_csv` as is? @bashtage @simonjayhawkins @WillAyd 
Closing this PR as I think still needs some discussion. If a point of interest please comment back on the original issue to get alignment on design approach