-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: ExcelWriter's engine and supported_extensions are properties #46444
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
Conversation
maybe they should all be variables, as at least pandas/pandas/io/excel/_util.py Line 46 in afec0e9
|
@rhshadrach Based on your recent PR to make |
@twoertwein - Yes, I'd lean toward a property rather than a method here. The most compelling reason to me is that keeps the API from changing. But this is a really good catch, I didn't realize the subtle inconsistency. |
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.
lgtm
Thanks @twoertwein! |
engine
andsupported_extensions
are declared to be properties inExcelWriter
but sub-classes simply assign variables to them. While the differences are subtle, we should either consistently use properties or variables (pyright doesn't like mixing them; mypy doesn't seem to care).There are more cases where a sub-class does not use a property (found by pyright when enabling reportGeneralTypeIssues):