Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

Refactor load_data for consistent sheet handling and cleaner code #488

Merged
merged 7 commits into from
Sep 21, 2023

Conversation

guyyanko
Copy link
Contributor

This PR addresses an issue in the load_data function when reading a specific sheet from an Excel file and performs code simplification.

Issues Addressed:

  1. Previously, reading a single sheet resulted in inconsistent behavior where the returned Document object contained a text char-by-char with \n between each.
  2. Redundant parentheses were found around self._row_joiner.

Changes Made:

  1. Modified how sheet_name is processed always to treat it as a list, ensuring consistent behavior across single or multiple sheet name inputs.
  2. Removed the unnecessary parentheses from self._row_joiner for cleaner code.
  3. Renamed the variable keys to sheet_names for better clarity.
  4. Updated the function's docstring to be more descriptive and accurate.

By implementing these changes, the function is now more robust and can handle different input scenarios effectively. Additionally, the code has been streamlined for better readability.

guy.yanko and others added 2 commits August 28, 2023 11:19
- Ensure sheet_name is always treated as a list to maintain consistent behavior
- Remove redundant parentheses from self._row_joiner
- Rename variable for clarity (from 'keys' to 'sheet_names')
- Update function docstring to reflect changes
@guyyanko
Copy link
Contributor Author

Hi @jerryjliu,
Is there anything missing for you to review the PR?
Let me know if anything should be done.
Thanks!

@jerryjliu
Copy link
Collaborator

@guyy1232 sorry for the delay! reviewing now

Copy link
Collaborator

@jerryjliu jerryjliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving this, though one comment. is it possible to keep include_sheetname as the variable? changing it might break backwards compatibility

@guyyanko
Copy link
Contributor Author

guyyanko commented Sep 7, 2023

approving this, though one comment. is it possible to keep include_sheetname as the variable? changing it might break backwards compatibility

Thank you for reviewing!
I renamed include_sheet_name back to "include_sheetname" for back compatibility (removed the "_")

Copy link
Contributor Author

@guyyanko guyyanko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed include_sheet_name back to "include_sheetname" for back compatibility (removed the "_")

@EmanuelCampos
Copy link
Collaborator

nice one

@EmanuelCampos EmanuelCampos merged commit 162575a into run-llama:main Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants