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

Handle multi row inline strings #464

Merged
merged 1 commit into from
Aug 10, 2019
Merged

Handle multi row inline strings #464

merged 1 commit into from
Aug 10, 2019

Conversation

mlh758
Copy link
Contributor

@mlh758 mlh758 commented Aug 7, 2019

This is based on #461 because I added to that unit test so either merge that one first and then this one, or just merge this one and close the other.

Description

Handle multi row inline strings, simplify the row combination logic and use a
string builder.

Related Issue

#462

Motivation and Context

Inline strings, not just shared strings, can have the r fields. Since the structs matched I went ahead and consolidated them.

How Has This Been Tested

Added unit test and verified against the sample file provided in the issue.

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.

@mlh758 mlh758 mentioned this pull request Aug 7, 2019
@xuri xuri self-assigned this Aug 8, 2019
@mlh758
Copy link
Contributor Author

mlh758 commented Aug 8, 2019

It looks like the tests go back to Go 1.9 and strings.Builder is unavailable. Do we still support Go 1.9? If so I can change that back to string concatenation.

@xuri
Copy link
Member

xuri commented Aug 9, 2019

The new version can no longer support Go 1.9. This change will be included in the next version 2.0.2. I suggest simply break compatibility and update README notes.

The inline string struct is actually the same
as the shared strings struct, reuse it.

Note that Go version 1.10 is required.

Fixes #462
@codecov-io
Copy link

Codecov Report

Merging #464 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
+ Coverage   96.38%   96.38%   +<.01%     
==========================================
  Files          24       25       +1     
  Lines        5254     5257       +3     
==========================================
+ Hits         5064     5067       +3     
  Misses        106      106              
  Partials       84       84
Impacted Files Coverage Δ
xmlSharedStrings.go 100% <100%> (ø)
rows.go 83.79% <100%> (-0.37%) ⬇️
sheet.go 96.51% <0%> (ø) ⬆️

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 e6b7ac9...acd7642. Read the comment docs.

@mlh758
Copy link
Contributor Author

mlh758 commented Aug 9, 2019

Okay I updated the readme to note that Go 1.10 or higher is required.

@xuri xuri merged commit adc4aed into qax-os:master Aug 10, 2019
@mlh758 mlh758 deleted the fix-462 branch October 29, 2019 17:32
@xuri xuri added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 6, 2020
nullfy pushed a commit to nullfy/excelize that referenced this pull request Oct 23, 2020
Fixed qax-os#462 Handle multi row inline strings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants