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.

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

extend cell value load to support custom datetime format #703

Merged
merged 11 commits into from
Oct 4, 2020
Merged

extend cell value load to support custom datetime format #703

merged 11 commits into from
Oct 4, 2020

Conversation

artiz
Copy link
Contributor

@artiz artiz commented Sep 10, 2020

PR Details

Add support for custom number formats to support all date-time variants parsing

Description

We are using excelize in our ETL project tp parse variety XLSX files from our customers and had found that lots of required dates are parsed incorrecty as raw numbers. So we decided to extend dates support to use custom formats

Related Issue

PR provides fix for #73

Motivation and Context

PR provides support of custom Excel datetime formats

How Has This Been Tested

New Unit Test added

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@xuri xuri added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 10, 2020
@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2020

Codecov Report

Merging #703 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #703      +/-   ##
==========================================
+ Coverage   95.52%   95.59%   +0.06%     
==========================================
  Files          31       31              
  Lines        8379     8442      +63     
==========================================
+ Hits         8004     8070      +66     
+ Misses        226      225       -1     
+ Partials      149      147       -2     
Impacted Files Coverage Δ
cell.go 97.60% <100.00%> (+0.86%) ⬆️
excelize.go 95.85% <100.00%> (ø)
file.go 93.24% <100.00%> (ø)
rows.go 95.00% <100.00%> (+0.15%) ⬆️
styles.go 100.00% <100.00%> (ø)
pivotTable.go 98.77% <0.00%> (+0.04%) ⬆️

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 89465f4...48047a6. Read the comment docs.

@artiz artiz changed the base branch from master to v2 September 10, 2020 16:35
@artiz artiz changed the base branch from v2 to master September 10, 2020 16:44
Copy link
Member

@xuri xuri left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, I have left some comments.

templates.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
Copy link
Member

@xuri xuri left a comment

Choose a reason for hiding this comment

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

In addition, I suggest removing personal info from the doc properties of the test spreadsheet files, and could you add some unit tests for the non covered codes?

@artiz
Copy link
Contributor Author

artiz commented Sep 22, 2020

In addition, I suggest removing personal info from the doc properties of the test spreadsheet files, and could you add some unit tests for the non covered codes?

Got it, will do

Copy link
Member

@xuri xuri left a comment

Choose a reason for hiding this comment

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

I have tested by the spreadsheet attachment 1_2017-06-29.xlsx in the issue #73 uploaded by vchugreev, It seems like the value 29\ June\ 2017\ \г\.;@ after applying number format code contains escape characters.

@artiz
Copy link
Contributor Author

artiz commented Sep 22, 2020

I have tested by the spreadsheet attachment 1_2017-06-29.xlsx in the issue #73 uploaded by vchugreev, It seems like the value 29\ June\ 2017\ \г\.;@ after applying number format code contains escape characters.

Fixed in https://github.com/artiz/excelize/commit/6e66d1dab42abf6bfce59685825e1ccd46bba2ad

@artiz artiz requested a review from xuri September 22, 2020 22:46
@omaralsoudanii
Copy link

Hi @artiz @xuri

Thank you for the amazing work you are doing, The library is pretty great when reading XLSX files, however I am facing the same problem when parsing dates.

Some times the date value is parsed as string e.g: 25/03/2016, some times it's parsed as a float like 43345.5 and some times as an integer.

I believe this PR would probably solve it? if so do you have any estimate when this will be merged?

Thanks!

@xuri xuri merged commit f2b8798 into qax-os:master Oct 4, 2020
@xuri
Copy link
Member

xuri commented Oct 4, 2020

@artiz Thanks for your PR. @omaralsoudanii I have merged this PR.

@artiz artiz deleted the extended-parse-time branch October 5, 2020 08:16
EugeneAndrosovPaser pushed a commit to ceearrashee/excelize that referenced this pull request Nov 14, 2020
* extend cell value load to support custom datetime format

* cleanup incorrect imports

* fix numeric values conversion as done in legacy Excel

* fix tests coverage

* revert temporary package name fix

* remove personal info from test XLSX files

* remove unused dependencies

* update format conversion in parseTime

* new UT to increase code coverage

* Resolve code review issue for PR qax-os#703

* Rename broken file name generated by unit test

Co-authored-by: xuri <xuri.me@gmail.com>
@xuri xuri mentioned this pull request Apr 20, 2021
jenbonzhang pushed a commit to jenbonzhang/excelize that referenced this pull request Oct 22, 2023
* extend cell value load to support custom datetime format

* cleanup incorrect imports

* fix numeric values conversion as done in legacy Excel

* fix tests coverage

* revert temporary package name fix

* remove personal info from test XLSX files

* remove unused dependencies

* update format conversion in parseTime

* new UT to increase code coverage

* Resolve code review issue for PR qax-os#703

* Rename broken file name generated by unit test

Co-authored-by: xuri <xuri.me@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants