Skip to content
This repository has been archived by the owner on May 21, 2022. It is now read-only.

A little editing #1

Closed
wants to merge 17 commits into from
Closed

A little editing #1

wants to merge 17 commits into from

Conversation

rootlch
Copy link

@rootlch rootlch commented Jul 7, 2012

Hi, I've made the code a little cleaner and a little easier to read. In addition, i've it wrote so that signing strings runs concurrently. This will improve its performance.

@dgrijalva
Copy link
Owner

Thanks for this. There are a lot of changes in this pull request. It might take me a bit to review.

@porjo
Copy link
Contributor

porjo commented Feb 20, 2014

2 years later... You weren't kidding! 😄

@dgrijalva
Copy link
Owner

Lol. I don't really know what to do with this PR. The commit comments aren't helpful and there aren't, as far as I can tell, functional changes. Have you looked through this, @porjo?

@dgrijalva dgrijalva closed this Mar 10, 2014
@dgrijalva
Copy link
Owner

I don't think I can land this for the following reasons:

  1. the commit comments are entirely unclear
  2. most of the changes are just coding style, without any clear indication of what changed or why
  3. the concurrent json encoding is almost certainly slower and is definitely more complicated
  4. any functional changes should include tests
  5. any changes purely for performance should include benchmarks

I'd be happy to address these changes in more detail as individual pull requests.

Waterdrips pushed a commit to Waterdrips/jwt-go that referenced this pull request Sep 14, 2020
…codeowners-1600076031644796752

[skip ci] Adding CODEOWNERS file
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants