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

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

Merged
merged 1 commit into from
Sep 10, 2023

Conversation

benedictjohannes
Copy link
Contributor

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.

In my go test -v does not report any check.
As such I think it would be better to switch to quicktest.
This commit switch all test but fuzzy_test to quicktest,
which is also already used in this library.
@ghost
Copy link

ghost commented Sep 10, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@benedictjohannes
Copy link
Contributor Author

benedictjohannes commented Sep 10, 2023

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

Copy link
Owner

@tealeg tealeg left a comment

Choose a reason for hiding this comment

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

Thank you for this. 3 very nice contributions today!

@tealeg tealeg merged commit fdb820f into tealeg:master Sep 10, 2023
2 checks passed
@benedictjohannes benedictjohannes deleted the switch-check-to-qt branch September 11, 2023 04:27
@benedictjohannes
Copy link
Contributor Author

@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
Copy link
Owner

tealeg commented Sep 11, 2023

@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
Copy link
Contributor Author

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:

  • Export low-level types/funcs in a separate package, such as those dealing with XML representations. The separate package should clearly state that although exported, such types/funcs are not intended to be used directly in most general use cases.
  • As such, stream writer or other performance enhancing "hacks" can be developed in a separate package in other repositories (maybe by other people), avoiding polluting the main package. An example use case would be a backend that exports data as Excel files, which doesn't need styling, table or graphs, but does need numformats. Or optionally SST, to generate smaller file sizes compared to repetitive string content with inlineStr.
  • The main xlsx package thus stay true to it's intended usage, free of unsafe "hacks", but still provide low level interfaces for those hackers to build performance enhanced versions.

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.

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.

2 participants