fix: getFileExtension() to Improve Readability and Handle Edge Cases. #8980
Conversation
|
💖 Thanks for opening this pull request! 💖 Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
damanV5
changed the title
damanV5
changed the title
|
@gkatsev , @heff , @mister-ben Could you please help with getting this merged? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8980 +/- ##
==========================================
+ Coverage 84.13% 84.16% +0.03%
==========================================
Files 120 120
Lines 8117 8116 -1
Branches 1948 1948
==========================================
+ Hits 6829 6831 +2
+ Misses 1288 1285 -3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for the PR! It's a nice simplification. It would consider com in https://example.com a file extensions too, but so does the original.
Could you add a test to test/until/utils/url.test.js with the type of URL that should now work?
I don't think "accept a training slash" is the best description. When I read that I thought it's intended for a URL like .../file.m3u8/. Something like to "match an extension without a filename"?
|
Sure @mister-ben . Let me add a test for this. I agree |
damanV5
changed the title
|
@mister-ben I have added test cases in |
| @@ -55,6 +55,8 @@ QUnit.test('should get the file extension of the passed path', function(assert) | |||
| assert.equal(Url.getFileExtension('foo/.bar/test.video.flv?foo=bar'), 'flv'); | |||
| assert.equal(Url.getFileExtension('http://www.test.com/video.mp4'), 'mp4'); | |||
| assert.equal(Url.getFileExtension('http://foo/bar/test.video.wgg'), 'wgg'); | |||
| assert.equal(Url.getFileExtension('http://www.test.com/video/.mp4'), 'mp4'); | |||
| assert.equal(Url.getFileExtension('http://www.test.com/video/.mp4/'), 'mp4'); | |||
There was a problem hiding this comment.
Sorry, my earlier comment wasn't to suggest we should support this trailing slash, just that the previous title sounded like this is was the PR was intended to do. I don't think we should add this if there is no precedent for it. The regex as it was before was good.
There was a problem hiding this comment.
@mister-ben The updated regex will also handle a trailing slash , which was not handled before.
As per your comment, I thought it would be great to handle trailing slash as well.
Eg. For urls like: test.com/mp4/ : it was previously returning "".
So now, this update
will return mp4 for this.
Though, let me know I can revert it if that is not required.
There was a problem hiding this comment.
I just checked the original regex also handled the trailing slash, but there was no test case written for that.
So I believe, we should keep supporting the trailing slash case.
Apologies for the confusion, but I'd like to clarify once more.
With this PR, the following case will now be handled :-
https://demo.unified-streaming.com/tears-of-steel.ism/.m3u8
Original Regex was returning "" for this particular case.
Summary:-
This PR is an attempt to handle more edge cases like above and improves the readability and simplicity of the getFileExtension() function.
Thanks !
Description
More robust edge case handling for getFileExtension().
Case : Type is not provided explicitly in {src,type} object.
In this case getFileExtension() is called to fetch the extension so as to get the type.
For certain cases, fn does not returns the correct extension.
For Eg:
In this case fn returns "".
I have simplified getFileExtension() so that fn will return m3u8 in this case as well.
Specific Changes proposed
Simplified getFileExtension() for better readability and to handle more edge cases.
Requirements Checklist
npm run docs:apito error