-
Notifications
You must be signed in to change notification settings - Fork 322
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
Add ExcelHandler #1962
Add ExcelHandler #1962
Conversation
Task linked: CU-86b05t4z0 SDV - Add ExcelHandler #1950 |
be88f8f
to
b01c582
Compare
f08f723
to
3318b12
Compare
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.
Looks good!
I just have 1 or 2 questions.
|
||
@patch('sdv.io.local.local.pd') | ||
def test_write(self, mock_pd): | ||
# Setup |
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.
Add test docstring
sdv/io/local/local.py
Outdated
"""A class for handling Excel files.""" | ||
|
||
def read(self, file_path, sheet_names=None): | ||
"""Read data from Excel files and returns it along with metadata. |
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.
return
sdv/io/local/local.py
Outdated
sheet_names = xl_file.sheet_names | ||
|
||
for sheet_name in sheet_names: | ||
data[sheet_name] = pd.read_excel( |
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.
Should we add a check that the sheet_name
exists in the file_path
here? Or it's ok to let pandas raise an error
pyproject.toml
Outdated
'xlsxwriter>=3.1.0', | ||
'openpyxl>=3.1.0' |
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.
After speaking with @npatki I think we should make these optional dependencies. I would also just refer to pandas' dependencies instead of maintaining this ourselves. Something like
[project.optional-dependencies]
excel_handler = [
'pandas[excel]'
]
3318b12
to
ab7585b
Compare
Resolves #1950
CU-86b05t4z0