kinow commented on pull request #116: URL: https://github.com/apache/commons-imaging/pull/116#issuecomment-958618064
>> If the JpegImageParser requires a parameter different than the TiffImageParser, I assume we would have to create a new JpegImagingParameter. In that case, wouldn't we have to keep the old constructor for backward compatibility until a new major release? > >I'm not totally sure what you mean here. Can you give an example? Sorry, I wasn't very clear. At the moment we have `JpegImageParser parser = new JpegImageParser();`. Internally, the `JpegImageParser` is using a `TiffImagingParameters`. But if we need to add a parameter that is used only by the `JpegImageParser`, I think we would want to have a `JpegImagingParameters`. In which case I am not sure if we would be able to maintain backward compatibility by replacing the `TiffImagingParameters` in the `JpegImageParser`. >since it makes it it kind of hard to tell what parameters to use when creating images of a certain type. :point_up: this is my main concern. Avoid users having to guess what's the parameter that they want to use when parsing a Jpeg, Tiff, Gif, etc. Preferably something that their IDE's can assist with, using auto-complete for example. >One idea for making this readily available in the API would be to add convenience factory methods in Imaging for creating instances of each parameters type. Ex: Possibly another design to consider/experiment :-) >Side note: Is there a reason the JpegImageParser uses TiffImageParameters? The two formats are not related, correct? Perhaps we could have a common base class for them? We can probably avoid that. I agree it doesn't make much sense for the parameters to have a relationship like that. Currently, [`JpegImageParser`](https://github.com/apache/commons-imaging/blob/0ccabc3416cecf25cdec90a522f0912baf19bc58/src/main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java#L751) has some code in common with the `TiffImageParser` for parsing EXIF metadata. I don't recall 100%, but I believe that's why I implemented everything in the `TiffImagingParameters` and, realizing the Jpeg parameters were the same, made the parent-child class relantionship. >Is the idea that it is confusing on what properties are copied over? Not at all, that part is clear and looks OK. It's with users being able to use the wrong parameter class by accident, and realizing only in runtime iff a runtime exception is thrown (or in the worst case, no exception is thrown and the program behaves in a wrong way; debugging an issue like that can be annoying I think). >I attempted to retain the null parameter functionality. In the normalizeParameters method, if a null parameter is passed, a default instance is created and returned, allowing downstream code to bypass null checks. mvn clean install passes with my current setup. Is there a unit test I missed? Ah, my bad! Your solution works with the method I was looking at. I wrote this one before looking at the method that creates the default parameters :+1: I need to find some time to “get in the zone” and try a few things. But it would be, mainly, to prevent users being able to use the wrong parameter types by accident, and making it so that we can add/remove parameters (deprecating what was removed) in the 1.x release series without worrying about backward compatibility. If we are able to modify your PR again to accommodate these two requirements, I think we would have the perfect solution :-) Thanks a lot! Bruno -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
