Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow toxlsx() to overwrite add or replace a worksheet #509

Merged
merged 9 commits into from
Aug 13, 2020
Merged

Allow toxlsx() to overwrite add or replace a worksheet #509

merged 9 commits into from
Aug 13, 2020

Conversation

juarezr
Copy link
Member

@juarezr juarezr commented Aug 7, 2020 •
edited

This PR is the same as #502 but rebased on top of #506

Changes

  1. Added new parameter mode to toxlsx() from $502
  2. Rebased on top of Allow reading and writing Excel files in remote sources #506
  3. Removed a call to os.path.exists() as it wont work with remote sources from fsspec

Checklist

  • Code
    • Includes unit tests
    • New functions have docstrings with examples that can be run with doctest
    • New functions are included in API docs
    • Docstrings include notes for any changes to API or behaviour
    • All changes documented in docs/changes.rst
  • Testing
    • (Optional) Tested local against remote servers
    • Travis CI passes (unit tests run under Linux)
    • AppVeyor CI passes (unit tests run under Windows)
    • Unit test coverage has not decreased (see Coveralls)
  • Changes
    • Ready to review
    • Ready to merge

@juarezr juarezr requested a review from alimanfoo August 7, 2020 23:19
@coveralls
Copy link

coveralls commented Aug 7, 2020 •
edited

Coverage Status

Coverage increased (+0.04%) to 91.96% when pulling a90cbf4 on juarezr:xlsx_add_sheet into d713928 on petl-developers:master.

@juarezr
Copy link
Member Author

juarezr commented Aug 11, 2020

@alimanfoo , @chrullrich ,

Everybody busy? 😄

chrullrich
chrullrich reviewed Aug 11, 2020
View reviewed changes
Copy link

@chrullrich chrullrich left a comment

Choose a reason for hiding this comment

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

Terribly busy, I'm afraid.

You imported a bug from me, and I cannot really speak to the remote code, but it looks to me like everything makes sense.

petl/io/xlsx.py Outdated Show resolved Hide resolved
petl/io/xlsx.py Show resolved Hide resolved
petl/test/io/test_remotes.py Outdated Show resolved Hide resolved
@juarezr juarezr added the Feature A nice to have thing that we don't have yet label Aug 13, 2020
@juarezr
Copy link
Member Author

juarezr commented Aug 13, 2020

Resuming this merge because:

  1. The review issues are addressed.
  2. There is no incompatible change in public API or existing behavior.
  3. We can make further changes if a problem arises.
  4. In the worst situation we can revert this back.
  5. Everybody is too busy for being bothered re-reviewing.

@juarezr juarezr merged commit f5b7022 into petl-developers:master Aug 13, 2020
This was referenced Aug 13, 2020
Closed
Closed
@juarezr juarezr deleted the xlsx_add_sheet branch September 17, 2020 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrullrich chrullrich chrullrich left review comments

@alimanfoo alimanfoo Awaiting requested review from alimanfoo

Assignees
No one assigned
Labels
Feature A nice to have thing that we don't have yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@juarezr @coveralls @chrullrich