kinow commented on code in PR #366: URL: https://github.com/apache/commons-imaging/pull/366#discussion_r1518986860
########## src/test/java/org/apache/commons/imaging/formats/tiff/write/TiffOutputFieldTestUtil.java: ########## @@ -0,0 +1,27 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.imaging.formats.tiff.write; + +public class TiffOutputFieldTestUtil { Review Comment: >There isn't a suitable other way to do so. @StefanOltmann I think you are worrying right now without need. There are multiple ways to do it. We can, for instance, do as in other parts of Imaging and expose `getBytes()` returning a copy of the private array. This is done in other formats/parsers. I found only one place where one of those comparison methods was being used in the code. >Another option would be to move the test into the same package. :+1: agreed, there are multiple options, but nothing to worry here. Discussing about that shouldn't be necessary here. It's old code from Sanselan, that needs some attention before we release 1.0 final version. There is nothing for you to worry, as there is nothing wrong in your code. No one is saying you changed too much, on the contrary, I told you you do not have to worry :slightly_smiling_face: I'm changing that because the code is old and instead of adding a new public method, or making it protected and adding an extra class, we may find other ways to solve it -- which is not pertinent to this. If we digress too much on discussions like this, the pull request will quickly lose momentum, the committer working (myself in this case) will have to work on other things, and your pull request may be left with no updates for a while again. So, please, do not worry about changing the code or justifying your changes, unless we request feedback for doing so :slightly_smiling_face: Your code is good, the API is old, the test looks great, just need to confirm the comments about offsets, and some changes you did in the test (which we do not need to discuss now, leave it be for a while until myself or another reviewer has had time to read it with calm) :+1: -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
