-
Notifications
You must be signed in to change notification settings - Fork 679
Fix hook extension case #2660
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
Fix hook extension case #2660
Conversation
src/io/image_reader_type.rs
Outdated
| .extension() | ||
| .filter(|ext| !ext.is_empty()) | ||
| .map(|ext| Format::Extension(ext.to_owned())); | ||
| .map(|ext| Format::Extension(ext.to_ascii_lowercase())); |
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.
Nit: Please add a comment on the enum's variant that its contents are supposed to be normalized to ascii lowercase.
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.
Instead of lower-casing here, I think it would be better to allow arbitrary casing in Format::Extension and lower-case in ImageReader::make_decoder where casing causes problems.
I'm saying this in the context of #2662 where I added a second code path that creates a Format::Extension.
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.
hm good point, that fits the existing pattern of ImageFormat::from_extension better. I don't want to affect that since your PR and issue 2616 might change it.
Make integration hook extensions case-insensitive, matching built-in formats. Also added some tests to show it failing before the change; not sure if they're useful or follow standards. New to Rust and first time contributing, so any feedback is welcome!
See issue #2659