BUG, DOC: Improve dialect handling in read_csv
State: closed

1) Update documentation about how the `dialect` parameter is handled.

2) Verify that the `dialect` parameter passed in is valid before accessing the dialect attributes.

Closes #14898.

Labels:
name: Docs
name: IO CSV
description: read_csv, to_csv

Comments:
## [Current coverage](https://codecov.io/gh/pandas-dev/pandas/pull/14911?src=pr) is 84.77% (diff: 100%)
> Merging [#14911](https://codecov.io/gh/pandas-dev/pandas/pull/14911?src=pr) into [master](https://codecov.io/gh/pandas-dev/pandas/branch/master?src=pr) will decrease coverage by **0.01%**

```diff
@@             master     #14911   diff @@
==========================================
  Files           145        145          
  Lines         51118      51129    +11   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits          43343      43343          
- Misses         7775       7786    +11   
  Partials          0          0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last update [caab85b...16e48bc](https://codecov.io/gh/pandas-dev/pandas/compare/caab85b8520d530c8c67a9e363c8f87905a456e8...16e48bcb00d4422e9ef8548519b93080f2d83860?src=pr)
lgtm.

@jorisvandenbossche 
I am not sure this is worth the discussion (I never user the dialect keyword, not sure if many people do), but, if we are adding this warning, I have the following problem (as also noted in the issue): 

- For certain cases the default value conflicts with the one specified in a dialect. In those cases you even get a warning when just doing `pd.read_csv(path, dialect='specific-dialect')`, which seems to defeat the purpose of the dialect a bit, i.e. to specify/override a bunch of arguments together.


@jorisvandenbossche : I am not sure I fully understand your concern.  What do you suggest then I do?   It's better to be safe than sorry if we have conflicting values.
> It's better to be safe than sorry if we have conflicting values.

At some point, yes. But there the `dialect` purpose *is* to override default values, so it would be strange that you get a warning for the *intended* use of the `dialect` kwarg. (if I misunderstand the use, that may well be, as I never use it). 
Eg:

```
In [1]: s = """a,b,c
   ...: 1,2,3"""

In [3]: import csv

In [4]: pd.read_csv(StringIO(s), dialect=csv.unix_dialect)
/home/joris/scipy/pandas/pandas/io/parsers.py:385: ParserWarning: Conflicting values for 'quoting': '0' was provided, but the dialect specifies '1'. Using the dialect-specified value.
  parser = TextFileReader(filepath_or_buffer, **kwds)
Out[4]: 
   a  b  c
0  1  2  3
```

So here the user did not provide `quoting=0` him/herself as the message would indicate, it was just the default. 

Maybe this is only a rare case, so we should not care about it too much. But the same could be said IMO for the warning itself which also adds complexity to the code (my saying: we could also limit it to just a clear documentation of the behaviour)
@jorisvandenbossche : `dialect=None` by default, so if somebody does pass in a `dialect` argument, they are of course _intending_ to use it.

However, to your point that we should only document this behavior, I am inclined to disagree at this point.  We currently warn about conflicts between `dtype` and `converters` for the C engine.  If you have conflicting values for a `read_csv` parameter, I don't see why that would be any different.
> We currently warn about conflicts between dtype and converters for the C engine. If you have conflicting values for a read_csv parameter, I don't see why that would be any different.

I agree that warning is good when the *user* provides two conflicting kwargs like dtype and converters (and we indeed do this!). But, my point is that in this case, it is not the user who provides them both, and as such has no control over it. It are the default values that raise the warning. 
And it is exactly the point of passing a `dialect` to override default values without having to pass each of those individual kwargs (and as a result, will give conflicts).
Yes, but how do you differentiate between passing the default argument in intentionally and not passing in any argument at all?  If you can't tell the difference, then you warn to be safe.  More information is better in such cases of uncertainty.
@gfyoung Just to be clear: ideally, we would be able to deduce whether a user has passed a conflicting kwarg themselves (eg `pd.read_csv(.., dialect="excel", quotechar=None)` where `quotechar=None` would be ignored due to `dialect="excel"`), and warn in such cases (that I would be totally be OK with). 

However, this is not possible in all cases, as we cannot distuinguish between a default value or the user passing a default value (in principle we could detect when the user passed a non-default value and only warn in those cases if there is a conflicting value. However, IMO, that is too much effort not worth this specific case.

A comment on the actual PR added docs: can you also update io.rst (with the explanation that you updated of the dialect kwarg)?
> Yes, but how do you differentiate between passing the default argument in intentionally and not passing in any argument at all? If you can't tell the difference, then you warn to be safe. More information is better.

See my follow-up comment I was already writing :-) 
More information is better -> good information is good, but unnecessary warnings are annoying IMO.
@jorisvandenbossche : Ah, yes, good catch.  `io.rst` updated as requested.
@jreback , @jorisvandenbossche : I have addressed all comments here.  Ready to merge if there are no other concerns.
lgtm. just that small comment (no big deal really).
Still, I am -1 on adding a warning that comes up for overriding default values (as argued above, this is the exact purpose of the keyword). But if nobody else makes the same objection, not going to argue.
I will disagree with @jorisvandenbossche here. I think the warning is useful (not that I have ever used a dialect!). But I would for sure want to know that I am overriding things.

(needs rebase as merged bunches of stuff).
To give a bit an extreme case. But suppose I specified my own custom dialect:

```
In [13]: class my_dialect(csv.Dialect):
    ...:     delimiter = '\t'
    ...:     quotechar = "'"
    ...:     doublequote = False
    ...:     skipinitialspace = False
    ...:     quoting = 2
```

So if I use this with read_csv:

```
 pd.read_csv(..., dialect=my_dialect)
```

this will generate (with this PR) a long warning indicating that I am overriding a lot of default values. If you want to get rid of those warnings, you have to do:

```
pd.read_csv(..., dialect=my_dialect, quotechar="'", doublequote=False, skipinitialspace=False, quoting=2)
```

so specifying them all individually to not override them. What is then the point of having the `dialect` kwarg with which you can override a bunch of keywords at once? (as this is the point of the keyword, to "override values (default or not)" (from the docstring)).

But as I never use dialect, I will not argue further:-) (which I actually now did ..). But I find it just introduction of a lot of code for something not useful.
@jorisvandenbossche : Fair enough - I guess the users shall have the final word :smile: 
ok lgtm. @gfyoung ping on green.
@jreback : Everything is green.  Ready to merge if there are no other concerns.
thanks!
getting this warning here: https://travis-ci.org/pandas-dev/pandas/jobs/193465661

```
>>>-------------------------------------------------------------------------
Warning in /tmp/doc/source/io.rst at block ending on line 426
Specify :okwarning: as an option in the ipython:: block to suppress this message
----------------------------------------------------------------------------
/home/travis/build/pandas-dev/pandas/pandas/io/parsers.py:389: ParserWarning: Conflicting values for 'quoting': '0' was provided, but the dialect specifies '3'. Using the dialect-specified value.
  parser = TextFileReader(filepath_or_buffer, **kwds)
<<<-------------------------------------------------------------------------
```

?