-
Notifications
You must be signed in to change notification settings - Fork 11
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
cwhisper: bug fixes and refactoring #23
Conversation
* refactor cmd/compare.go * refactor cmd/dump.go * refactor tests * fix ooo write for single retention cwhisper files #20
It all looks good to me, and yeah, I agree that overall codebase of go-whisper is sometimes rather hard to correct, I guess that's why it's like that. |
a7b1d1b
to
7914a7f
Compare
@Civil haha, it's mostly complaining compress.go. It's mostly the style ones are left unhandled, some of them were not really introduced in this PR so I'm not very enthusiastic to have them corrected. |
I like static code analysers, but they are not replacement for human eyes ofc. I would treat them only as recommendation. |
I'm fine leaving these analysis issues unfixed. |
Got it, thanks for all your comments! @dgryski haha, I have to say I'm a bit surprised that you also have an open attitude toward analysis issues, given that you have shown a strong favor of linter and things in your tweets and works. :D |
Yeah, but some fixes are just code churn and don't provide value. I think the remaining issues fall into that category. (Although I did notice |
Should I merge it, @bom-d-van ? |
@deniszh oh, thanks for reminding me. Yep, just pushed error message fix suggested by Damian. Should be ready to merge now. |
Some of the changes were ported from a pr 18, which I decide to give up due to the challenges/problems of supporting both out of order writes and aggregation.
As always, all changes shouldn't affect standard whisper file reads/writes.