CesiumGS / gltf-pipeline

Content pipeline tools for optimizing glTF assets. :globe_with_meridians:
Apache License 2.0
1.91k stars 250 forks source link

use case-insensitive regexes for checking image extensions #270

Closed likangning93 closed 7 years ago

likangning93 commented 7 years ago

See https://github.com/AnalyticalGraphicsInc/gltf-pipeline/blob/master/lib/loadGltfUris.js#L160

Instead of using a list of supported extensions and checking using indexof, we should use a regular expression when checking if the format is supported.

var ext = // some image format
var supportedExtensions = /\.(png|jpg|jpeg|bmp)$/i;
supportedExtensions.test(ext);

Thanks @mramato!

HappyDash commented 7 years ago

Another easy way to club jpeg and jpg together in regex.

var ext = // some image format var supportedExtensions = /^.*.(jpe?g|png|bmp)$/i; supportedExtensions .test(ext);

ryki2658 commented 7 years ago

I would like to help with this. It will be my first contribution to a open source project. I have forked and cloned the repository, I have made the changes that have been requested (...at least I have attempted to). Do you want me to now add commit and push the changes?

lilleyse commented 7 years ago

@ryki2658 Yes go ahead and then open a pull request.

Also since this is your first contribution you will need to sign a CLA before we merge any code: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/CONTRIBUTING.md#contributor-license-agreement-cla

ryki2658 commented 7 years ago

Ok I sent the agreement over and I opened a pull request.

emackey commented 7 years ago

@HappyDash @ryki2658 Would the regex behave any differently if you dropped ^.* from the front? Why explicitly specify "starts with... anything" ?

HappyDash commented 7 years ago

@emackey extensions start with dot So I thought this regex will clearly specify that required string should always start with dot

mramato commented 7 years ago

Long story short: /\.(png|jpe?g|bmp)$/i is the best option here.

@HappyDash your regex is actually incorrect because it does not verify the extension starts with a period. See the below diagram (source): image You need to escape the period in order to have the regex check for it.

Contrast that to /\.(png|jpg|jpeg|bmp)$/i; which produces a simpler workflow: image

So the original example is correct, but you are right in that it can be slightly shorter with /\.(png|jpe?g|bmp)$/i : image

emackey commented 7 years ago

@mramato The regex was fixed in 7ab14d94b20dbf5cf27 in #278.

HappyDash commented 7 years ago

Oh yeah I am really sorry, I didn't check it properly. Actually I have just learned writing some regexes at office. I didn't see it through. Thanks, for explaining it properly. Also, diagrams are so cool.

On May 8, 2017 7:11 PM, "Ed Mackey" notifications@github.com wrote:

@mramato https://github.com/mramato The regex was fixed in 7ab14d9 https://github.com/AnalyticalGraphicsInc/gltf-pipeline/commit/7ab14d94b20dbf5cf27d3ef7d7ace22de6978744 in #278 https://github.com/AnalyticalGraphicsInc/gltf-pipeline/pull/278.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/AnalyticalGraphicsInc/gltf-pipeline/issues/270#issuecomment-299869917, or mute the thread https://github.com/notifications/unsubscribe-auth/ANoansd0V9d1-cgHb_u6Ncp7N7FQlylFks5r3xt7gaJpZM4NLdNn .