-
Notifications
You must be signed in to change notification settings - Fork 621
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
feat(path/unstable): support URL
in extname()
#5818
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5818 +/- ##
=======================================
Coverage 96.25% 96.25%
=======================================
Files 479 479
Lines 38697 38748 +51
Branches 5613 5621 +8
=======================================
+ Hits 37246 37298 +52
+ Misses 1409 1408 -1
Partials 42 42 ☔ View full report in Codecov by Sentry. |
extname()
URL
in extname()
path/posix/extname.ts
Outdated
export function extname(path: URL): string; | ||
export function extname(path: string | URL): string { | ||
if (path instanceof URL) { | ||
path = path.pathname; |
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.
Can you use fromFileUrl
instead? This allows other random urls such as http:
, data:
, jsr:
, node:
, etc, which is not aligned to how dirname
works with those url types.
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.
What's the harm in allowing these other URL schemes?
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.
In data:
scheme, the pathname doesn't represent the path, but it represent arbitrary data. The return value of extname
can be any random value in that case.
*/ | ||
export function extname(path: URL): string; | ||
export function extname(path: string | URL): string { | ||
// deno-lint-ignore no-explicit-any |
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.
Couldn't any
be avoided with toString()
or String(path)
?
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.
We like to do path instanceof URL
check in each implementation. String(path)
makes that check not working.
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.
LGTM
Towards #5537