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

refactor(utils/basic-auth): Moved Internal function to utils #3359

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

sugar-cat7
Copy link
Contributor

@sugar-cat7 sugar-cat7 commented Sep 2, 2024

I have moved the functions used internally in the Basic Auth Middleware to the utils directory. Additionally, I have added tests.

For the background on this refactor, please refer to the following: honojs/middleware#676 (comment)

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code
  • Add TSDoc/JSDoc to document the code

const USER_PASS_REGEXP = /^([^:]*):(.*)$/
const utf8Decoder = new TextDecoder()

export type Auth = (req: HonoRequest) => { username: string; password: string } | undefined
Copy link
Member

Choose a reason for hiding this comment

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

I prefer making req as Request not HonoRequest. The utility function should be more general. So, HonoRequest that depends on Hono is not good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review.
I have removed the dependency on HonoRequest.
989a0db

@yusukebe
Copy link
Member

yusukebe commented Sep 2, 2024

Hi @sugar-cat7

Thank you for the PR(this is your first PR for honojs/hono!). I've left a comment. Check it.

Copy link

codecov bot commented Sep 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.77%. Comparing base (bdaaa7f) to head (989a0db).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3359      +/-   ##
==========================================
+ Coverage   95.76%   95.77%   +0.01%     
==========================================
  Files         151      152       +1     
  Lines        9186     9187       +1     
  Branches     2708     2699       -9     
==========================================
+ Hits         8797     8799       +2     
+ Misses        389      388       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe
Copy link
Member

yusukebe commented Sep 3, 2024

@sugar-cat7

Thank you! I'm merging it.

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.

2 participants