Skip to content

feat: Use klauspost/compress for gzip#14

Open
li-jin-gou wants to merge 1 commit into
mainfrom
optimize/gzip
Open

feat: Use klauspost/compress for gzip#14
li-jin-gou wants to merge 1 commit into
mainfrom
optimize/gzip

Conversation

@li-jin-gou
Copy link
Copy Markdown
Collaborator

@li-jin-gou li-jin-gou commented Oct 29, 2023

What type of PR is this?

feat

What this PR does / why we need it (English/Chinese):

  • zh: 使用 klauspost/compress 代替标准库
  • en: use klauspost/compress instead of standard

Which issue(s) this PR fixes:

@li-jin-gou li-jin-gou force-pushed the optimize/gzip branch 3 times, most recently from d72ad35 to 5f304cb Compare November 12, 2023 14:48
@li-jin-gou
Copy link
Copy Markdown
Collaborator Author

Hello @klauspost,I copied https://github.com/cloudwego/hertz/tree/develop/pkg/common/compress over and changed the gzip dependency, but the original single test was not used. Hope you can help me look at the unit test when you have time, thank you very much.

@klauspost
Copy link
Copy Markdown

@li-jin-gou The test seems to expect the gzip package to produce a specific compressed output. That is a wrong expectation.

While you can change the expected output, it will break again. So either A) check the decompressed output or B) Check that the size isn't 5% bigger than you expect. Exact compressed size will change over time as the algorithm is improved.

@li-jin-gou
Copy link
Copy Markdown
Collaborator Author

The test seems to expect the gzip package to produce a specific compressed output. That is a wrong expectation.

While you can change the expected output, it will break again. So either A) check the decompressed output or B) Check that the size isn't 5% bigger than you expect. Exact compressed size will change over time as the algorithm is improved.

Thanks

Comment thread client_middleware.go
Comment thread gzip_test.go
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.

3 participants