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

cwhisper: bug fixes and refactoring #23

Merged
merged 3 commits into from
Sep 6, 2020
Merged

Conversation

bom-d-van
Copy link
Member

@bom-d-van bom-d-van commented Jul 26, 2020

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.

* refactor cmd/compare.go
* refactor cmd/dump.go
* refactor tests
* fix ooo write for single retention cwhisper files #20
@bom-d-van bom-d-van marked this pull request as ready for review August 27, 2020 13:28
@bom-d-van bom-d-van self-assigned this Aug 27, 2020
@bom-d-van
Copy link
Member Author

Hi @deniszh @Civil , I have tried to fix some of the deepsource failures, but the rest of them are kinda hard/sad to correct, if you disagree and find any of the existing ones should definitely be fixed, please let me know.

@Civil
Copy link
Member

Civil commented Aug 27, 2020

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.

@bom-d-van
Copy link
Member Author

@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.

@deniszh
Copy link
Member

deniszh commented Aug 27, 2020

I like static code analysers, but they are not replacement for human eyes ofc. I would treat them only as recommendation.

@dgryski
Copy link
Member

dgryski commented Aug 27, 2020

I'm fine leaving these analysis issues unfixed.

@bom-d-van
Copy link
Member Author

bom-d-van commented Aug 27, 2020

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

@dgryski
Copy link
Member

dgryski commented Aug 27, 2020

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 err = fmt.Errorf("Unable to read header: %s", err.Error()) and just having err instead of err.Error() is sufficient, since %s will call Error() for you).

@deniszh
Copy link
Member

deniszh commented Sep 6, 2020

Should I merge it, @bom-d-van ?
Or you want to add something?

@bom-d-van
Copy link
Member Author

@deniszh oh, thanks for reminding me. Yep, just pushed error message fix suggested by Damian. Should be ready to merge now.

@bom-d-van bom-d-van merged commit 8c4f3a7 into master Sep 6, 2020
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.

4 participants