Skip to content

Update xlsw.py: fix timedelta support #49

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

Merged
merged 4 commits into from
Oct 2, 2021
Merged

Update xlsw.py: fix timedelta support #49

merged 4 commits into from
Oct 2, 2021

Conversation

vinraspa
Copy link
Contributor

@vinraspa vinraspa commented Oct 2, 2021

fix timedelta support

With your PR, here is a check list:

  • Has test cases written?
  • Has all code lines tested?
  • Has make format been run?
  • Please update CHANGELOG.yml(not CHANGELOG.rst)
  • Has fair amount of documentation if your change is complex
  • Agree on NEW BSD License for your contribution

fix timedelta support
W291 trailing whitespace: fixed
Bad cell ref [0, 2] -> [0, 3]
@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2021

Codecov Report

Merging #49 (8f7e6f7) into dev (e309f9e) will increase coverage by 1.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev      #49      +/-   ##
==========================================
+ Coverage   97.42%   98.42%   +1.00%     
==========================================
  Files          13       13              
  Lines         698      697       -1     
==========================================
+ Hits          680      686       +6     
+ Misses         18       11       -7     
Impacted Files Coverage Δ
pyexcel_xls/xlsr.py 99.24% <ø> (ø)
pyexcel_xls/xlsw.py 98.30% <100.00%> (+11.20%) ⬆️
tests/test_formatters.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b774fc...8f7e6f7. Read the comment docs.

@chfw
Copy link
Member

chfw commented Oct 2, 2021

I am happy that you fix the problem. But does it make sense to support timedelta? So can you describe your use case?

@chfw chfw merged commit 680731d into pyexcel:dev Oct 2, 2021
@vinraspa
Copy link
Contributor Author

vinraspa commented Oct 3, 2021

I write a python program that must export ical events to a spreadsheet.
Events have a begin and end times (datetime) and duration (timedelta) fields.
I want user to be able to play with these fields (sum, min, max, ...)

Currently, with pyexcel module:

  • xlsx format supports both datetime and timedelta (this is cool and this proves it's possible)
  • xls format supports datime but not timedelta
  • ods format supports timedelta but not datetime
    That is why I made PRs to pyexcel-xls, pyexcel-ods3 and pyexcel-io projects.

@chfw
Copy link
Member

chfw commented Oct 3, 2021

I see. But xlrd returns date time for timedelta. How do you translate it back to date time?

@vinraspa
Copy link
Contributor Author

vinraspa commented Oct 3, 2021

Yes, this is an issue
The conversion works only one way (python->xls) but not the other.
It would require xlrd deal differently when time format contains [H] or [HH] (instead of anything else).
I will think about it a bit.

Is it worth it? maybe you are right. After all, xls format is less and less used.

@chfw
Copy link
Member

chfw commented Oct 3, 2021

Xls has a limit on the number of rows so it is better to avoid using xls. UK’s national health service used xls format to persist collected reported COVID cases in 2020 and got a mystery problem: where did the data go? Oh well, xls skipped excessive rows.

@chfw
Copy link
Member

chfw commented Oct 3, 2021

I think we can release the change but note it in doc. Of course, If you are able to work out the remedying solution, and it can be added to the doc.

@vinraspa
Copy link
Contributor Author

vinraspa commented Oct 6, 2021

By 'the doc', do you mean 'README.rst'?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants