-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
[CLN] Excel Module Cleanups #25275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CLN] Excel Module Cleanups #25275
Changes from all commits
7bbe910
c98f72e
bc074ed
62ce7db
5e49439
cf3c5bc
e42b10c
a82119a
7f81675
03bc237
f30ab5c
9ef747f
19160fa
bcf053e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,32 +5,39 @@ | |
|
||
from pandas.core.dtypes.common import is_integer, is_list_like | ||
|
||
from pandas.core import config | ||
|
||
_writer_extensions = ["xlsx", "xls", "xlsm"] | ||
|
||
|
||
_writers = {} | ||
|
||
|
||
def register_writer(klass): | ||
"""Adds engine to the excel writer registry. You must use this method to | ||
integrate with ``to_excel``. Also adds config options for any new | ||
``supported_extensions`` defined on the writer.""" | ||
""" | ||
Add engine to the excel writer registry.io.excel. | ||
|
||
You must use this method to integrate with ``to_excel``. | ||
|
||
Parameters | ||
---------- | ||
klass : ExcelWriter | ||
""" | ||
if not callable(klass): | ||
raise ValueError("Can only register callables as engines") | ||
engine_name = klass.engine | ||
_writers[engine_name] = klass | ||
for ext in klass.supported_extensions: | ||
if ext.startswith('.'): | ||
ext = ext[1:] | ||
if ext not in _writer_extensions: | ||
config.register_option("io.excel.{ext}.writer".format(ext=ext), | ||
engine_name, validator=str) | ||
_writer_extensions.append(ext) | ||
|
||
|
||
def _get_default_writer(ext): | ||
""" | ||
Return the default writer for the given extension. | ||
|
||
Parameters | ||
---------- | ||
ext : str | ||
The excel file extension for which to get the default engine. | ||
|
||
Returns | ||
------- | ||
str | ||
The default engine for the extension. | ||
""" | ||
_default_writers = {'xlsx': 'openpyxl', 'xlsm': 'openpyxl', 'xls': 'xlwt'} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would make this a module level variable, but maybe needs a followup for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could see this going either way, so agreed that a followup makes sense. |
||
try: | ||
import xlsxwriter # noqa | ||
|
@@ -230,8 +237,6 @@ def _fill_mi_header(row, control_row): | |
|
||
return _maybe_convert_to_string(row), control_row | ||
|
||
# fill blank if index_col not None | ||
|
||
|
||
def _pop_header_name(row, index_col): | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to be clear this was never part of the public API hence why I think OK to remove. If a user in spite of that still registered a custom subclass with an extension outside of
.xls
,.xlsx
or.xlsm
then potentially it would have an impact, but that seems like such an edge case to me hence why I think OK to remove.cc @jreback in any case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm...I'm -0.5 on this. Discarding because it's an "edge case" does not seem like a strong argument for removing this code if we're in the business of robustness. Potentially there are better ways to implement this, but I wouldn't necessarily go as far as removing for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main point is that it's not part of the public API and is a no-op with current internal code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I'm still a little on the fence on this though for the reason I gave earlier, so getting @jreback input would be helpful for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is ok i think