[sonos] Exchanged file extensions to existing constants #3598
Conversation
Signed-off-by: Christoph Weitkamp <[email protected]>
| } else if (AudioFormat.MP3.isCompatible(format)) { | ||
| handler.playNotificationSoundURI(new StringType(url + ".mp3")); | ||
| handler.playNotificationSoundURI(new StringType(url + FileAudioStream.MP3_EXTENSION)); |
There was a problem hiding this comment.
I really don't know, but isn't it a little bit unexpected that an "extension" constant contains the "filename-extension-separator" (the dot)?
There was a problem hiding this comment.
Hm ... probably not.
The constant(s) is/are used in the getAudioFormat(File file) method to determine the audio format. Maybe we should refactor those lines to compare file extensions using the FilenameUtils.isExtension(String filename, String extension) method from Apache Commons IO. And additionally use FilenameUtils.EXTENSION_SEPARATOR_STR to concat again. wdyt?
There was a problem hiding this comment.
I mean ... probably yes. Which changes the statement of my reply.
There was a problem hiding this comment.
Wikipedia uses this summary for Filename extension:
A filename extension is an identifier specified as a suffix to the name of a computer file. The extension indicates a characteristic of the file contents or its intended use. A file extension is typically delimited from the filename with a full stop (period), but in some systems it is separated with spaces.
We can also life with the fact that our extension constants contains the separator already but I don't think this is a good practice to use stuff different then expected.
The constant(s) is/are used in the getAudioFormat(File file) method to determine the audio format. Maybe we should refactor those lines to compare file extensions using the FilenameUtils.isExtension(String filename, String extension) method from Apache Commons IO. And additionally use FilenameUtils.EXTENSION_SEPARATOR_STR to concat again. wdyt?
I would at least expect that a filename, a filename extension and the separator are handled separately and not mixed.
If Apache Commons IO is already used by our bundles, I am fine with using constants and methods it provides. Otherwise we can also fix it in our code base without Commons IO.
If we drop the dot from the extension then we will potential break some bindings...
@kaikreuzer WDYT about using the common name handling in our code base?
There was a problem hiding this comment.
Sure, this should be changed.
I'd prefer doing it without Apache commons though - the less dependencies we have, the better.
Signed-off-by: Christoph Weitkamp <[email protected]>
Signed-off-by: Christoph Weitkamp [email protected]