-
Notifications
You must be signed in to change notification settings - Fork 665
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
refactor: rename FreshContext.req
to FreshContext.request
#2593
base: main
Are you sure you want to change the base?
refactor: rename FreshContext.req
to FreshContext.request
#2593
Conversation
@marvinhagemeister Ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At Deno here we prefer ctx.req
for brevity reasons. Merging this would make my coworkers unhappy. Instead of removing it entirely, you could add a getter to alias ctx.request
-> ctx.req
.
Are you sure? It seems there are lot of upvotes which agree with the change (#2363 (comment) |
@timreichen I don't find Because let's be real, this PR is only nitpicking and personal preference. Deep down it's not important what the name of the variable is as long as it's somewhat clear. I use Hono a lot, it's Apart from you apparently. |
I don't find But this is not about personal preference, it is about consistency and alignment with web apis. What I find would be a personal preference however is to sacrifice consistency over the convenience of saving a few chars. |
@marvinhagemeister I think we should resolve this issue soon. I worked a lot on deno @std for the last two years helping to make the mods consistent among each other and follow web api syntax and conventions. So this issue seems like a no-brainer to me. Is there any chance it can be adopted? |
Apart from the consistency concerns, I think "req" instead of "request" is also bad because:
In many of the points above, I speak from personal experience. On several occasions, I’ve had to remind work colleagues, non-technical product owners, or even my own tired 3am brain what "req", "res", "opt", "dep", "acc", etc. mean at some line deep in a codebase. And that is where you lose actual time. In our newer codebases, I introduced the aforementioned eslint plugin and code reviews are now 20% more fun (a made up stat, but I hope you get my point). |
@marvinhagemeister I added the alias with a deprecation note. I think that way a slow adoption can happen during the alpha releases and before 1.0 is released. WDYT? |
ref: #2363 (comment)
FreshContext .req
property toFreshContext.request
@marvinhagemeister