Skip to content

Comments

Fix confusing names#579

Merged
peterbourgon merged 4 commits intomasterfrom
fix-confusing-names
Jul 19, 2017
Merged

Fix confusing names#579
peterbourgon merged 4 commits intomasterfrom
fix-confusing-names

Conversation

@peterbourgon
Copy link
Member

Closes #526.

@ChrisHines @basvanbeek — interested in a review?

@willfaught @terinjokes — likewise, and FYI.

@peterbourgon
Copy link
Member Author

peterbourgon commented Jul 19, 2017

For the record,

#!/usr/bin/env fish

for pair in \
    "FromHTTPContext ContextToHTTP" \
    "ToHTTPContext   HTTPToContext" \
    "FromGRPCContext ContextToGRPC" \
    "ToGRPCContext   GRPCToContext" \
    "FromHTTPRequest HTTPToContext" \
    "ToHTTPRequest   ContextToHTTP" \
    "FromGRPCRequest GRPCToContext" \
    "ToGRPCRequest   ContextToGRPC"
  set fr (echo $pair | awk '{print $1}')
  set to (echo $pair | awk '{print $2}')
  echo change from $fr to $to
  find . -type f -name '*.go' -exec sed -i '' s/$fr/$to/ \{\} + # macOS syntax
end

Copy link
Member

@ChrisHines ChrisHines left a comment

Choose a reason for hiding this comment

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

The code looks good. I think the docs have some broken grammar.

)

// ToHTTPContext moves JWT token from request header to context. Particularly
// HTTPToContext moves JWT token from request header to context. Particularly
Copy link
Member

Choose a reason for hiding this comment

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

I feel obligated to point out that JWT is an acronym for JSON Web Token, which makes the phrase "JWT token" repetitive. I believe a more grammatically correct phrasing is "... moves a JWT from request header to context."

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Thank you.

)

// ToHTTPContext moves JWT token from request header to context. Particularly
// HTTPToContext moves a JWT from request header to context. Particularly
Copy link
Contributor

Choose a reason for hiding this comment

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

The "a" article use is now inconsistent (present with "JWT", absent with "request header" and "context"), not sure if that's intentional, not a big deal.

// HTTPToContext moves a JWT from request header to context. Particularly
// useful for servers.
func ToHTTPContext() http.RequestFunc {
func HTTPToContext() http.RequestFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

"Context" seems to be in all of these names, so is it redundant? Would To/FromHTTP and To/FromGRPC be equally clear? I haven't taken a close look at the whole file, and I've never used the package, so I'm genuinely asking. :) If not, the names look good!

@peterbourgon peterbourgon merged commit 1beb0cb into master Jul 19, 2017
@peterbourgon peterbourgon deleted the fix-confusing-names branch July 19, 2017 16:55
jamesgist pushed a commit to jamesgist/kit that referenced this pull request Nov 1, 2024
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