-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
streaming writer set default value like non-streaming one #2152
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
Conversation
Change-Id: Idd8da825025a3ab046d74380ec36f25091c41560 Signed-off-by: mengzhongyuan <[email protected]>
Change-Id: If019aea806b9527937a9fe8d90050d2ef3d783b0 Signed-off-by: mengzhongyuan <[email protected]>
6da209b
to
d83ae68
Compare
…riter Change-Id: I7b29a679603217d81e566ddc42567a5c369f6bc7 Signed-off-by: mengzhongyuan <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2152 +/- ##
=======================================
Coverage 99.23% 99.23%
=======================================
Files 32 32
Lines 30273 30278 +5
=======================================
+ Hits 30042 30047 +5
Misses 153 153
Partials 78 78
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@xuri ptal, this is test suite for our biz already. |
Sure, I need to take some time to review this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the streaming writer by adding support for setting a cell's default value similarly to the non‑streaming writer and unifies receiver naming for consistency.
- Added support for a new SetCellDefaultValue type in the streaming writer flow.
- Updated test cases including expected cell counts and row indices to reflect the new behavior.
- Renamed receiver variables in rows.go for uniformity in methods handling cell XML attributes and elements.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
stream_test.go | Updated tests to exercise SetCellDefaultValue and adjusted expected cell count. |
stream.go | Added handling for SetCellDefaultValue in setCellValFunc. |
rows.go | Unified receiver naming from “cell” to “c” for consistency. |
cell.go | Introduced SetCellDefaultValue type and modified setCellDefault logic. |
Comments suppressed due to low confidence (2)
cell.go:588
- Clarify the rationale behind setting c.T to an empty string for default cell values; consider expanding the comment to reference the intended behavior difference between streaming and non-streaming writers.
c.T, c.V, c.IS = "", "", nil // I modify this to set c.T = "" explicitly to avoid misunderstanding of c.Type can be value
stream_test.go:157
- [nitpick] Add a comment to explain the updated expected cell count, clarifying how the new SetCellDefaultValue rows affect the total cell count in the test.
assert.Equal(t, 2559468, cells)
There was a problem hiding this 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 don't think this changes is required.
The cell value with the string
data type will be storage as inlineStr
in the normal mode SetCellDefault
function, the SetCellValue
and SetCellStr
function will storage cell value as shared string table. That's the major different between them.
But in the stream writer, the SetRow
function always storage string
data type cell value as inlineStr
, the behavior same as normal mode SetCellDefault
function, so we needn't introduce new SetCellDefaultValue
for this.
@@ -408,6 +414,8 @@ func TestStreamSetCellValFunc(t *testing.T) { | |||
true, | |||
nil, | |||
complex64(5 + 10i), | |||
SetCellDefaultValue("100.1588"), | |||
SetCellDefaultValue(" Hello"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If set specific characters, it will caused corrupted workbook. For example:
err := sw.SetRow("A1", []interface{}{excelize.SetCellDefaultValue("<>"))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (c *xlsxC) setCellDefault(value string) {
if ok, _, _ := isNumeric(value); !ok {
if value != "" {
c.setInlineStr(value)
c.IS.T.Val = value
return
}
c.T, c.V, c.IS = "", "", nil // I modify this to set c.T = "" explicitly to avoid misunderstanding of c.Type can be value
return
}
c.T, c.V = "", value
}
I think the difference between setCellDefault and setCellString is the former can set c.T to a empty string, i didn't find the path to do that using streaming writer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cell type is empty only for no-numeric cell values, here an example for different data type of set cell values with SetCellDefault
and stream mode function:
package main
import (
"fmt"
"github.com/xuri/excelize/v2"
)
func main() {
f := excelize.NewFile()
defer func() {
if err := f.Close(); err != nil {
fmt.Println(err)
}
}()
if err := f.SetCellDefault("Sheet1", "A1", "100.1588"); err != nil {
fmt.Println(err)
return
}
if err := f.SetCellDefault("Sheet1", "B1", "<>"); err != nil {
fmt.Println(err)
return
}
_, err := f.NewSheet("Sheet2")
if err != nil {
fmt.Println(err)
return
}
sw, err := f.NewStreamWriter("Sheet2")
if err != nil {
fmt.Println(err)
return
}
if err := sw.SetRow("A1", []interface{}{100.1588, "<>"}); err != nil {
fmt.Println(err)
return
}
if err := sw.Flush(); err != nil {
fmt.Println(err)
return
}
if err := f.SaveAs("Book1.xlsx"); err != nil {
fmt.Println(err)
}
}
the generated XML element of Sheet1!A1 is same as Sheet2!A1, and Sheet2!B1 is same as Sheet2!B1:
<row r="1">
<c r="A1">
<v>100.1588</v>
</c>
<c r="B1" t="inlineStr">
<is>
<t><></t>
</is>
</c>
</row>
So you can using stream writer to generats same things that same as SetCellDefault
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Got it, you are right. I will check it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've closed this, if you have any questions, please let me know, and I can reopen this anytime.
PR Details
Related Issue
#2151
Motivation and Context
We have a legacy impl using the non-streamingly writer to write excel, in which we use setCellDefault func to set raw value.
Now i want to migrate some logic to streaming writer impl, i found no a corresponding impl setCellDefault for steaming write. :)
How Has This Been Tested
Types of changes
Checklist