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

remove unnecessary for loop in token signing string for readability #34

Merged
merged 3 commits into from Feb 3, 2022

Conversation

hyeonjae
Copy link
Contributor

remove unnecessary for-loop in token signing string for readability
and ...

  • add testcase
  • add benchmark
  • improve performance slightly

 - add testcase
 - add benchmark
 - improve performance slightly
Copy link
Collaborator

@oxisto oxisto left a comment

Choose a reason for hiding this comment

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

Thanks. Indeed improves readability a little bit. I wonder if there was any trick to the initial implementation that I am not seeing at first glance here.

token_test.go Show resolved Hide resolved
Copy link
Member

@lggomez lggomez left a comment

Choose a reason for hiding this comment

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

Looks good, needs an update from main and the requested change to make the benchmark to be more precise and report the allocations, otherwise I think we're fine (also see https://github.com/golang-jwt/jwt/pull/34/files#r680371882)

token_test.go Show resolved Hide resolved
token_test.go Show resolved Hide resolved
token_test.go Outdated Show resolved Hide resolved
token_test.go Outdated Show resolved Hide resolved
@lggomez lggomez requested a review from oxisto January 16, 2022 20:26
@lggomez
Copy link
Member

lggomez commented Jan 16, 2022

This is another stale code refactor that we'd want to have in v4 and before other refactors like #115

LGTM. Care to give it a check @oxisto? since the PR author didn't respond these last months I commited the purposed changes to make this benchmarkable on v4

@oxisto
Copy link
Collaborator

oxisto commented Feb 3, 2022

This is another stale code refactor that we'd want to have in v4 and before other refactors like #115

LGTM. Care to give it a check @oxisto? since the PR author didn't respond these last months I commited the purposed changes to make this benchmarkable on v4

I was still cautions to touch functions that are the very core of this library, but it looks sound, doesn't add more allocations and is marginally faster, so I think you can merge it.

@lggomez lggomez merged commit e01ed05 into golang-jwt:main Feb 3, 2022
oxisto pushed a commit to moneszarrugh/jwt that referenced this pull request Feb 21, 2023
…olang-jwt#34)

* remove unnecessary for loop in token signing string for readability

 - add testcase
 - add benchmark
 - improve performance slightly

* Fix benchtests on token_test.go

* Update token_test.go to v4

Co-authored-by: hyeonjae <hyeonjae@ip-192-168-1-3.ap-northeast-2.compute.internal>
Co-authored-by: Luis Gabriel Gomez <lggomez@users.noreply.github.com>
oxisto pushed a commit to twocs/jwt that referenced this pull request Mar 29, 2023
…olang-jwt#34)

* remove unnecessary for loop in token signing string for readability

 - add testcase
 - add benchmark
 - improve performance slightly

* Fix benchtests on token_test.go

* Update token_test.go to v4

Co-authored-by: hyeonjae <hyeonjae@ip-192-168-1-3.ap-northeast-2.compute.internal>
Co-authored-by: Luis Gabriel Gomez <lggomez@users.noreply.github.com>
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.

None yet

4 participants