tealeg / xlsx

Go library for reading and writing XLSX files.
Other
5.81k stars 808 forks source link

switch gopkg.in/check.v1 to quicktest in some test files #775

Closed benedictjohannes closed 10 months ago

benedictjohannes commented 10 months ago

In my go test -v, some tests (like in date_test.go) that uses gopkg.in/check.v1 isn't being performed, even with check.v flag.

As such I think it would be better to switch to quicktest. This commit/PR switch all tests coded with check.v1 to quicktest except those in fuzzy_test.go.

Result of the switch shows that there are more lines of report (which indicates number of tests being performed).

> go version
go version go1.21.1 linux/amd64
> go test -v | wc -l
1390
> git checkout switch-check-to-qt 
Switched to branch 'switch-check-to-qt'
Your branch is up to date with 'origin/switch-check-to-qt'.
> go test -v | wc -l
1464

Attached are test run results for tests switched to quicktest and the original / master branch.

ghost commented 10 months ago
👇 Click on the image for a new way to code review #### [![Review these changes using an interactive CodeSee Map](https://s3.us-east-2.amazonaws.com/maps.codesee.io/images/github/tealeg/xlsx/775/a690e567/0e6ed53f1eba8e4c2475aaf879ababf5467907fb.svg)](https://app.codesee.io/r/reviews?pr=775&src=https%3A%2F%2Fgithub.com%2Ftealeg%2Fxlsx) #### Legend CodeSee Map legend
benedictjohannes commented 10 months ago

If commit in this PR and my PR to fix RoundTripFileWithNoSheetCols test is being merged, all tests passed.

benedictjohannes commented 10 months ago

@tealeg Thanks man, that's very swift review and merging.

I'm perplexed that this you mark this library as no longer maintained

This library has much better type awareness in reading/writing Excel cells than excelize you suggest in README. As such that library receives much more attention, while it provide no interface to access a single Cell such that with a single reference we can easily get cell content, type, numFormat, etc (instead those are accessed using fileRef.GetCell[Type|Value|Style|Formula].

However I noticed Excel datetime parsing accuracy issues. I'll be working to fix it.

tealeg commented 10 months ago

@benedictjohannes - it's marked that way because I basically got burned out on the whole thing. At one point I had co-maintainers, and I even stepped back and let other people run the project, but everyone ran out of steam eventually. There are things that got added to the project that in retrospect I shouldn't have accepted, as they expanded the scope in clever, but hacky ways.

I'm actively toying with the idea of taking that statement away and saying we're open for contributions, but I'm not currently developing new features, which is basically the state. What do you think?

benedictjohannes commented 10 months ago

There are things that got added to the project that in retrospect I shouldn't have accepted, as they expanded the scope in clever, but hacky ways.

I think you're likely refering to streaming excel writer that was removed in v3.
I observed excelize's benchmark that shows excelize library having better performance compared to this, with their stream writer using much smaller RAM.

I'd have to admit that I'm interested in performance enhancements, but I'd do it differently:

I'm actively toying with the idea of taking that statement away and saying we're open for contributions, but I'm not currently developing new features, which is basically the state. What do you think?

I think that is truly an excellent idea. I'd say a maintainer doesn't have to be developing new features (or even contributing in new code) but should review contributions, accepting good ones, and steer the course of the project.