Updated read_excel docstring to match style guide formatting #53953

GarrettDaniel · 2023-06-30T20:32:05Z

Fixes read_excel() docstring to match docstring formatting style guide:

rhshadrach

Thanks for the PR! Overall looks good - some requests.

rhshadrach · 2023-07-03T12:13:33Z

pandas/io/excel/_base.py

@@ -79,7 +79,7 @@
    )
 _read_excel_doc = (
    """
-Read an Excel file into a pandas DataFrame.
+Read an Excel file into a ``pandas`` ``DataFrame``.


I'm thinking we shouldn't wrap DataFrame throughout. In my opinion, it doesn't need the highlighting, and it can serve as a distraction.

Good call. I wasn't sure on that one so I'm glad you provided some clarity on it. It could go either way, but I agree that it gets distracting with how often DataFrame is referenced in the docs.

rhshadrach · 2023-07-03T12:18:05Z

pandas/io/excel/_base.py

+    - ``"xlrd"`` supports old-style Excel files (.xls).
+    - ``"openpyxl"`` supports newer Excel file formats.
+    - ``"odf"`` supports OpenDocument file formats (.odf, .ods, .odt).
+    - ``"pyxlsb"`` supports Binary Excel files.


I would suggest no double quotes here (so just e.g. ``xlrd``). We can just be referring to the engine itself, rather than the string argument.

Good call! I'll make sure to change that in the commit I'm working on.

rhshadrach · 2023-07-03T12:20:06Z

pandas/io/excel/_base.py

-    Detect missing value markers (empty strings and the value of na_values). In
-    data without any NAs, passing na_filter=False can improve the performance
+    Detect missing value markers (empty strings and the value of ``na_values``). In
+    data without any NAs, ``passing na_filter=False`` can improve the performance


passing shouldn't be included (just na_filter=False)

Nice catch!

rhshadrach · 2023-07-03T12:21:10Z

pandas/io/excel/_base.py

-    arrays, nullable dtypes are used for all dtypes that have a nullable
-    implementation when "numpy_nullable" is set, pyarrow is used for all
-    dtypes if "pyarrow" is set.
+dtype_backend : {{"numpy_nullable", "pyarrow"}}, defaults to ``numpy`` backed ``DataFrames``


NumPy is the correct capitalization, now?

It is, I just wasn't sure if it would be better to go with NumPy or numpy, but like the comment above with DataFrame and pandas, I agree that it would be distracting to make every instance of NumPy a code block.

rhshadrach · 2023-07-03T12:22:53Z

pandas/io/excel/_base.py

@@ -356,7 +356,7 @@
 1       NaN      2
 2  #Comment      3

-Comment lines in the excel input file can be skipped using the `comment` kwarg
+Comment lines in the excel input file can be skipped using the ``comment`` ``kwarg``


kwarg here should not be included. Can you write out the full phrase here: keyword argument.

Good call. Should comment still be in a code block in this case since it's referencing the name of a parameter?

Yes - I think so.

rhshadrach · 2023-07-03T12:25:11Z

pandas/io/excel/_base.py

-    parse some cells as date just change their type in Excel to "Text".
-    For non-standard datetime parsing, use ``pd.to_datetime`` after ``pd.read_excel``.
+    parse some cells as date, just change their type in Excel to "Text".
+    For non-standard ``datetime`` parsing, use ``pd.to_datetime`` after ``pd.read_excel``.


I don't think datetime here is being used in a technical sense (e.g. specifying a package or snippet of code), and so shouldn't highlighted.

Good point. If it was referencing the datetime package or dtype that would be a more appropriate time to put it in a codeblock.

rhshadrach · 2023-07-03T12:26:49Z

pandas/io/excel/_base.py

@@ -345,7 +345,7 @@
 1   string2    2.0
 2  #Comment    3.0

-True, False, and NA values, and thousands separators have defaults,
+``True``, ``False``, ``NaN`` values, and thousands of separators have defaults,


The docs use NA as opposed to NaN in various places because it can refer to pd.NA. Can you revert this change.

rhshadrach · 2023-07-03T12:30:57Z

pandas/io/excel/_base.py

    * If callable, then evaluate each column name against it and parse the
      column if the callable returns ``True``.

    Returns a subset of the columns according to behavior above.
 dtype : Type name or dict of column -> type, default None
-    Data type for data or columns. E.g. {{'a': np.float64, 'b': np.int32}}
+    Data type for data or columns. E.g. ``{'a': np.float64, 'b': np.int32}``


I believe you need to leave the double curly braces {{ and }} for jinja-style templating.

I do agree that we need that for jinja-style templating, but from what I can tell, we aren't using jinja-style templating in this case. If we were, wouldn't we be passing in parameters or referencing an external file to be read in and rendered like a formatted string? (https://realpython.com/primer-on-jinja-templating/). At the moment, it renders like a regular string, but this seems like a perfect use case for a code block.

Agreed on using the code-block for this, but I believe the change to the curly braces is making the CI fail; e.g.:

https://github.com/pandas-dev/pandas/actions/runs/5426779826/jobs/9869261284?pr=53953#step:9:54

Ok that's good to know. In that case, should we just leave it to render as a string with the double curly braces so that CI won't fail? I suppose we could put that in a code block but it might look a little strange (i.e. {{'a': np.float64, 'b': np.int32}})

(i.e. {{'a': np.float64, 'b': np.int32}})

When you do this, do both curly braces render in the docs? I would expect only one renders.

I've been trying to get the docs to render as per the guide (https://pandas.pydata.org/docs/development/contributing_documentation.html#building-the-documentation), but I keep running into ModuleNotFoundError: No module named 'pandas.__libs.pandas_parser' when running python3 make.py html in the doc directory. This is most likely because of the version of pandas I have on my laptop, but I tried upgrading to the newest version and uninstalling + reinstalling and the error still persists. At this point, I'll commit it with the code block around the braces and if the CI build still fails I'll just remove the code block and leave it to render as standard text.

Did you create a development environment and compile/install pandas?

https://pandas.pydata.org/docs/development/contributing_environment.html

I've tried a few of the different methods on that doc, namely with Docker, DevContainers, and Mamba, and none of them were able to successfully import pandas. I believe it has something to do with a corporate proxy or security issue, but I either run into that same ModuleNotFoundError, or I run into ERROR: Disabling PEP 517 processing is invalid: project specifies a build backend of mesonpy in pyproject.toml when trying to run python -m pip install -e . --no-build-isolation --no-use-pep517. If you have any ideas on this, let me know. Otherwise, I've made all of the other requested changes in the other comments.

I believe you're going off the stable (e.g. 2.0.3) version of the docs for building pandas. When reading docs on development, it's best to read the dev docs as we will break dev-specific things well before releasing 😆. See here:

https://pandas.pydata.org/pandas-docs/dev/development/contributing_environment.html#step-3-build-and-install-pandas

Otherwise, I've made all of the other requested changes in the other comments.

The CI is failing; this will need to be addressed (which is easiest when you can build locally).

mroeschke · 2023-08-11T01:04:35Z

Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.

Updated read_excel docstring to match style guide formatting

71babd9

GarrettDaniel requested a review from rhshadrach as a code owner June 30, 2023 20:32

rhshadrach requested changes Jul 3, 2023

View reviewed changes

Changes referenced in PR review comments from @rhshadrach

9e46790

mroeschke added the IO Excel read_excel, to_excel label Jul 6, 2023

mroeschke closed this Aug 11, 2023

linus-md mentioned this pull request Dec 11, 2023

DOC: Improve io/excel/_base.py #56453

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated read_excel docstring to match style guide formatting #53953

Updated read_excel docstring to match style guide formatting #53953

GarrettDaniel commented Jun 30, 2023

rhshadrach left a comment

rhshadrach Jul 3, 2023

GarrettDaniel Jul 3, 2023

rhshadrach Jul 3, 2023

GarrettDaniel Jul 3, 2023

rhshadrach Jul 3, 2023

GarrettDaniel Jul 3, 2023

rhshadrach Jul 3, 2023

GarrettDaniel Jul 3, 2023

rhshadrach Jul 3, 2023

GarrettDaniel Jul 3, 2023

rhshadrach Jul 3, 2023

rhshadrach Jul 3, 2023

GarrettDaniel Jul 3, 2023 •

edited

Loading

rhshadrach Jul 3, 2023

GarrettDaniel Jul 3, 2023

rhshadrach Jul 3, 2023

GarrettDaniel Jul 3, 2023 •

edited

Loading

rhshadrach Jul 3, 2023

GarrettDaniel Jul 3, 2023 •

edited

Loading

rhshadrach Jul 3, 2023

GarrettDaniel Jul 3, 2023 •

edited

Loading

rhshadrach Jul 5, 2023

GarrettDaniel Jul 13, 2023 •

edited

Loading

rhshadrach Jul 15, 2023 •

edited

Loading

rhshadrach Jul 15, 2023 •

edited

Loading

mroeschke commented Aug 11, 2023

Updated read_excel docstring to match style guide formatting #53953

Updated read_excel docstring to match style guide formatting #53953

Conversation

GarrettDaniel commented Jun 30, 2023

rhshadrach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GarrettDaniel Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GarrettDaniel Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GarrettDaniel Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GarrettDaniel Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GarrettDaniel Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

rhshadrach Jul 15, 2023 • edited Loading

Choose a reason for hiding this comment

rhshadrach Jul 15, 2023 • edited Loading

Choose a reason for hiding this comment

mroeschke commented Aug 11, 2023

GarrettDaniel Jul 3, 2023 •

edited

Loading

GarrettDaniel Jul 3, 2023 •

edited

Loading

GarrettDaniel Jul 3, 2023 •

edited

Loading

GarrettDaniel Jul 3, 2023 •

edited

Loading

GarrettDaniel Jul 13, 2023 •

edited

Loading

rhshadrach Jul 15, 2023 •

edited

Loading

rhshadrach Jul 15, 2023 •

edited

Loading