DOC: Updating the docstring of read_csv and related functions

[thoo]

• closes DOC: Fix docstring of read_csv and related methods #23496
• tests added / passed
• passes git diff upstream/master -u -- "*.py" | flake8 --diff

Make docstrings of the following modules to comply with pandas docstring

• pandas.read_table
• pandas.read_csv
• pandas.read_fwf

[pep8speaks]

Hello @thoo! Thanks for submitting the PR.

• There are no PEP8 issues in the file pandas/io/parsers.py !

• Complete extra results for this file :

file_to_check.py:310:-6532: W605 invalid escape sequence '\s'

---

[datapythonista]

Nice fixes. I think besides fixing the parameters, that look great, we need examples and see also sections for these docstrings, right?

Sorry it's a bit tricky to find out myself the way those docstrings are created.

[datapythonista]

Suggested change

	float_precision : str, default None
	float_precision : str, optional

[thoo]

@datapythonista Should I write a file like object to memory using io.StringIO() for the examples?

[datapythonista]

The best option we found for showing functions that save to disk is df.to_csv('/tmp/data.csv') # doctest: +SKIP.

I'm not sure when reading, I think there is a directory with some files that are used for that. Can you take a look? Or may be @TomAugspurger can help.

[TomAugspurger]

Not sure what's best here. We could make an HTTP request to https://github.com/pandas-dev/pandas/blob/master/doc/data/tips.csv, but I'd rather avoid that on every test run. I'm fine with just skipping.

[datapythonista]

Suggested change

	result : DataFrame or TextParser"""
	DataFrame or TextParser"""

[datapythonista]

Suggested change

	Alternative argument name for sep.
	Alias for sep.

[codecov]

Codecov Report

Merging #23517 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #23517      +/-   ##
==========================================
- Coverage   92.29%   92.29%   -0.01%     
==========================================
  Files         161      161              
  Lines       51493    51486       -7     
==========================================
- Hits        47524    47517       -7     
  Misses       3969     3969

Flag	Coverage Δ
#multiple	90.68% <100%> (-0.01%)	⬇️
#single	42.31% <100%> (-0.01%)	⬇️

Impacted Files	Coverage Δ
pandas/io/parsers.py	95.55% <100%> (-0.02%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99df7da...766d73f. Read the comment docs.

[thoo]

@datapythonista Could you re-review this? So should I leave {**kwds} errors from pandas.read_fwf for now?? Thanks.

[datapythonista]

Looks good, just added couple of things I saw, not sure if they are right.

[datapythonista]

If I'm not missing something, in the 3 functions reusing _parser_params this same _see_also is used.

Can you check if that's the case, and if it is move the content inside _parser_params? Also, it'd be nice to have the 3 functions self-reference themselves, and also the equivalent to_* functions.

[thoo]

I add more in See also section to cross-reference each other except read_table due to being deprecated in 0.24.0. I don't think we have to_table or to_fwf other than to_csv.

[datapythonista]

Added some ideas to make the rendering of the docstrings simpler. I think it was extremely complex for what it could be.

[datapythonista]

Sorry I wasn't clear before. What I'd do is add read_csv and read_fwf here, so we remove a bit of the complexity in the rendering. Referencing a function to itself is not ideal, but I think it'd better than having too much complexity in the code.

[datapythonista]

May be I'm missing something, but I think we should be able to do:

Suggested change

	%s # doctest: +SKI"""
	>>> pd.{func_name}('data.csv') # doctest: +SKIP"""

Besides removing the need to _example_doc, note also the >>>, the extra space before #, the missing P in SKIP, and I think I suggested myself using /tmp/, but as we're skipping it anyway, it's probably clearer for the user just using data.csv.

[datapythonista]

I think it'd be better to name all the %s. Having them positional ames things a bit difficult to follow, at least for me.

[datapythonista]

I think it would make more sense to have {summary} in the first line of _parser_params than this.

Sorry to be picky will all these, but I'd really want to simplify all this. The code of these functions is already very complex, having all this extra complexity for the docstrings makes things much worse IMO. I know it's from the original code, thanks for the help cleaning them.

[jreback]

yeah changing these to use named paramaters with .format() would be good

[datapythonista]

Really nice work, much much clearer now.

Just couple of small things that IMO would still improve the docstring. The first is renaming _parser_params to _parser_doc, as I think it better represents the content now.

Then, in the line 711 and around, the docstring is injected to the functions. I'd do the rendering of the docstring there, instead of creating intermediate variables _summary_read_csv and _read_csv_doc:

read_csv = _make_parser_function('read_csv', default_sep=',')
read_csv = Appender(_parser_doc.format(
    func_name='read_csv'
    summary='Read a comma-separated values (csv) file into DataFrame.',
    sep_doc=_sep_doc.format(default="','"),
    engine_doc=_engine_doc)
)(read_csv)

Feel free to disagree if you don't think that's an improvement, but I think it makes the code neater, and things easier to find.

Thanks!

[datapythonista]

Changes look great.

I just think this is not being used anymore. Can you check please? And also run the validate_docstrings.py on the 3 docstrings edited here, to see that everything is ok? I guess the script should be reporting that these params are missing in the docstring.

[thoo]

Yes. It is not used anymore. I run both validate_docstrings.py and python make.py --single, and they all look fine except {**kwds} error from read_fwf.

[datapythonista]

That sounds good. There is something that just by the code I don't understand. I see the signature of read_fwf is def read_fwf(filepath_or_buffer, colspecs='infer', widths=None, **kwds):, but we're using the docstring in _parser_doc that has lots of other parameters like header... That doesn't seem correct. Is the script not complaining of unknown parameters?

Or I'm missing something?

[thoo]

I forgot to add _fwf_widths back to read_fwf. But the script is still complaining of unknow parameters.

Parameters {**kwds} not documented
        Unknown parameters {decimal, float_precision, ...

I thought there will be a fix in the test to accommodate {**kwds}.

[datapythonista]

**kwds should also be documented, explaining why there are there (they are usually passed to another function). Any reason for not documenting them?

We can leave that for another PR if it's not trivial to add.

[thoo]

I added the doc for **kwds.
The only error left is from read_fwf:

1 Errors found:
        Unknown parameters {usecols, index_col, warn_bad_lines, chunksize, float_precision, false_values, na_values, skiprows, names, infer_datetime_format, parse_dates, delimiter, nrows, thousands, compression, escapechar, dialect, error_bad_lines, dayfirst, low_memory, header, skipfooter, quotechar, verbose, keep_date_col, converters, prefix, lineterminator, doublequote, skipinitialspace, mangle_dupe_cols, true_values, memory_map, quoting, decimal, comment, dtype, encoding, skip_blank_lines, iterator, delim_whitespace, tupleize_cols, na_filter, date_parser, keep_default_na, squeeze}

Is there anything I could do?

[datapythonista]

read_fwf signature doesn't have all the parameters in the shared docstring, see its signature: read_fwf(filepath_or_buffer, colspecs='infer', widths=None, **kwds):

I don't think we should be reusing the docstring of the others for it, or have all those parameters in a separate variable that is only added in the other docstrings.

The point here is that the docstring for every function/method has to have the right information. You can render the documentation of read_fwf with ./doc/make.html --single=pandas.read_fwf. If you check the html, you should see that we're documenting parameters that the function doesn't have, and that obviously doesn't make sense.

[datapythonista]

I think only these two functions read_csv and read_table use _parser_params now. So, if that's the case, and both have the same value for _engine_doc, I think it makes more sense to have the content directly inside _parser_params. And not have this variable anymore, right?

Other than that looks good to me. I think now the validation should be happy with the 3 functions, right?

[thoo]

I also remove _sep_doc variable and merge it inside _parser_params. Let me know if you want the other way. Now the validation is fine with all these three 👍 .

[datapythonista]

Moving the common sep_doc stuff into _parser_params sounds like a good idea.

I just realized while writing this that _parser_params is not a good name anymore. Do you mind renaming it to _doc_read_csv_and_table or something like that?

Other than that looks good to me.

[datapythonista]

Looks good, made suggestions for changes to the types, as we'll start validating them more strictly soon, but other than that looks good. If you can also remove one unnecessary item from the See Also, and let's merge afterwards.

[datapythonista]

Didn't see that before, can you remove this line? As this docstring is only used for read_fwf, there is no value in self-referencing itself.

[datapythonista]

Suggested change

	dialect : str or csv.Dialect instance, default None
	dialect : str or csv.Dialect, optional

[datapythonista]

Suggested change

	thousands : str, default None
	thousands : str, optional

[datapythonista]

Suggested change

	chunksize : int, default None
	chunksize : int, optional

[datapythonista]

Suggested change

	skiprows : list-like or int or callable, default None
	skiprows : list-like, int or callable, optional

[datapythonista]

Suggested change

	converters : dict, default None
	converters : dict, optional

[datapythonista]

Suggested change

	index_col : int or sequence or False, default None
	index_col : int, sequence or bool, default None

[datapythonista]

Suggested change

	header : int or list of ints, default 'infer'
	header : int, str or list of int, default 'infer'

[datapythonista]

Suggested change

	widths : list of ints. optional
	widths : list of int, optional

[datapythonista]

Suggested change

	colspecs : list of pairs (int, int) or 'infer'. optional
	colspecs : list of tuple (int, int) or 'infer'. optional

[datapythonista]

Thanks for the update @thoo, looks perfect now.

[jreback]

thanks @thoo pls check the rendered dev docs for correctness when they are built..