Hello Gilles!
I was interested in hearing your opinion as you commented in the previous thread too. >[Hmm... Does "internal" mean that minor release can break BC >on such a class?] I think so, yes. [1] is a commit to a temporary branch, where I renamed the .util package to internal, and updated the package-info.java, with a note. I am thinking about using that commit regardless of what's decided with the logging, so that users will be warned about the Debug class usage. Also added notes to Javadocs of Debug. >[Depends on what "low-level" means here. "stdout"/"stderr" is >indeed used in low-level utilities but is the intent the same >here?] I don't think so. My use case for Commons Imaging is to use it in the IIIF server Cantaloupe, to test and compare with other image libraries, with life sciences images. As the application will be running in the web, I would like to have proper logging, or at least be able to decide where and weather it is logged. >> which means removing the verbose flags, the checks, and calls to >> System.out in there. > >It would be a loss of potentially useful information (e.g. for >debugging). Indeed. Probably we should be looking for the alternatives that keep the existing verbose/logging... but probably allowing for more customisation I guess... Thanks a lot Gilles! Bruno [1] https://github.com/kinow/commons-imaging/commit/736fd115c12642ecabb3b4ab73df83b1e5982076 ________________________________ From: Gilles <gil...@harfang.homelinux.org> To: dev@commons.apache.org Sent: Monday, 13 August 2018 12:21 AM Subject: Re: [imaging] IMAGING-154 remove Debug class Hello Bruno. On Sun, 12 Aug 2018 08:56:37 +0000 (UTC), Bruno P. Kinoshita wrote: > Hi all, > > > I commented on IMAGING-154, but copying the last comment here as it > contains the approach I would like to follow to fix the last change > in > the project blocking > a 1.0 release: > > > --- > > So went ahead to re-design the Debug class, in a way users could > still enable/disable debugging, and also use a PrintStream so that > other thing rather than System.out could be used. > > > > Then, realized removing System.out was the natural next step. But > alas, the library uses System.out for debugging, but sometimes it > uses > it for writing to System.out in a "verbose mode". What is more > complicated, is that sometimes classes methods like `toString()` are > calling debug methods that receive a PrintStream already. > > > > So I spent some more time quickly comparing what other libraries I've > seen being used / or used for image processing: > > https://kinoshita.eti.br/2018/08/12/use-of-logging-in-java-image-processing-libraries/. > Turns out only very low level libraries, such as the JNI bridge for > OpenCV, im4java, and Java's ImageIO can do with just throwing > Exception's. > > > > All other libraries have one way or another of logging. Be it with > JUL, SLF4J, custom loggers, or with the ol' System.out/err. > > > > My preferred compromise for this ticket was to keep Debug, making > System.out possible but optional, and mark the class internal only. > Now my preferred solution is to keep the Debug internal, but add a > logger to it. And then also add logging to replace where System.out > is > used for the "verbose" mode. > --- > > > > Any thoughts? Objections? If none, I will try to work on this issue > next weekend, making the Debug class internal only, and replacing > System.out by a logging utility. After that, we should be good to > start preparing the vote for 1.0. > > > > * I know it's hard to get a consensus on having logging in Commons > components, as we have normally low level libraries, where using > logging is not always practical. There are Log4j2 experts reading here. It would be interesting to hear them about what is practical or not. There are several aspects to "practical": simplicity, flexibility, compatibility, performance, ... How does Log4j2 fare in these areas? Is there a known (through experience) limit in where it should be used? > But I would now argue that Java own > ImageIO is low level. But ImageJ2, Processing, OpenJPEG, and Commons > Imaging are located at a higher level, some times even using it for > basic image handling/parsing/reading. As with many discussions on this list, conflicting arguments occur because people lack common (!) definitions. So one goes: "You cannot do <something> in a low-level component" but does not define "low-level"... > * Feel free to cast a counter-argument for it, but please think > whether you'd still be -0, +0 for this change. We have delayed 1.0 > for > a while, so if you have a strong opinion on not adding a logger, > please provide an alternative for IMAGING-154. > Otherwise we may fail > to prepare a 1.0 release yet again, and then somebody else may have > to > work on it in a few months/years... We are there because the project is too rigid about itself as a whole (cf. for example the [RNG] thread about BC). IMHO, it's not the always least common denominator that is the best decision... As you noticed, components most easily stall in their development for lack of proper review, or risk acceptance (i.e. assume that those who are closer to the code (at a given time) probably know best... :-/ My opinion is that we can take the risk to introduce logging, and if people complain somehow, take it back later. > one possible compromise for this, > might be i) make Debug internal, +1 [Hmm... Does "internal" mean that minor release can break BC on such a class?] > ii) remove all System.out calls, +1 or -1 [Depends on what "low-level" means here. "stdout"/"stderr" is indeed used in low-level utilities but is the intent the same here?] > which means removing the verbose flags, the checks, and calls to > System.out in there. It would be a loss of potentially useful information (e.g. for debugging). Regards, Gilles > > Thanks! > Bruno > > ________________________________ > From: Bruno P. Kinoshita <brunodepau...@yahoo.com.br.INVALID> > To: Commons Developers List <dev@commons.apache.org> > Sent: Tuesday, 6 February 2018 11:30 PM > Subject: Re: [imaging] IMAGING-154 remove Debug class > > > > Hi sebb, > >>Another aspect of debugging is ensuring that methods are small and > >>easily tested independently. >>However this is difficult to do, and care must be taken to ensure >> that >>the public API is not unnecessarily extended.. > > A very good point. > > The parsers in commons-imaging expose some #dump... methods > > (https://github.com/apache/commons-imaging/blob/7e7f96857df999175bb614732e13272a82f7962a/src/main/java/org/apache/commons/imaging/ImageParser.java#L794). > > While I can see that parsers may need to dump the data they are > holding in some structured way for inspecting, reporting, > serializing, > etc, it looks like some other classes were affected by it too. For > example... > > > A JPEG Segment has a #dump() method > > > > https://github.com/apache/commons-imaging/blob/7e7f96857df999175bb614732e13272a82f7962a/src/main/java/org/apache/commons/imaging/formats/jpeg/segments/Segment.java#L34 > > > which gets defined in each subclass of Segment. It can be confusing > to have a method such as #dump() in a Segment, from the point of view > of someone writing a photo editor for example. The user could use > that > to pass his/her own logger's PrintWriter, which would make removing > or > changing logging in the future in commons-imaging. > > > If we keep the Debug class, and make it internal, there would still > be these methods to take care. And there are some methods where users > can provide a PrintWriter, while others instead use System.out > > (e.g.https://github.com/apache/commons-imaging/blob/7e7f96857df999175bb614732e13272a82f7962a/src/main/java/org/apache/commons/imaging/FormatCompliance.java#L70). > > Cheers > Bruno > > ________________________________ > From: sebb <seb...@gmail.com> > To: Commons Developers List <dev@commons.apache.org>; Bruno P. > Kinoshita <brunodepau...@yahoo.com.br> > Sent: Tuesday, 6 February 2018 11:06 PM > Subject: Re: [imaging] IMAGING-154 remove Debug class > > > > On 6 February 2018 at 09:52, Bruno P. Kinoshita > <brunodepau...@yahoo.com.br.invalid> wrote: >> Hi Jorg, >> >> I'd be fine with that solution too. I think this one would cause the >> smaller change to the code as is. >> >> I believe my preference is still to remove the Debug class. But >> between logging and making Debug internal only, I'd choose making it >> internal. > > +1 > > I think making it internal means it can still be dropped later. > >> Looking forward to hearing what others think about these options. >> > > Another aspect of debugging is ensuring that methods are small and > easily tested independently. > However this is difficult to do, and care must be taken to ensure > that > the public API is not unnecessarily extended.. > >> Thanks >> Bruno >> >> >> ________________________________ >> From: Jörg Schaible <joerg.schai...@bpm-inspire.com> >> To: dev@commons.apache.org >> Sent: Tuesday, 6 February 2018 9:24 PM >> Subject: Re: [imaging] IMAGING-154 remove Debug class >> >> >> >> Hi Bruno, >> >> >> if it might also be helpful to our users, why not keep and provide >> it. As >> >> I understand it, the Debug class is a tool helping in development to >> >> analyze some behavior. >> >> >> Nothing stops us from declaring this class internal (we might even >> put it >> >> into a package "internal" or "debug") that might be changed without >> >> further comment. Nobody may rely on it in production code, but >> during >> >> development it might be helpful. With such an approach we might not >> have >> >> a need to find a better interface to provide this functionality. >> >> >> Just my 2¢, >> >> Jörg >> >> >> >> Am Mon, 05 Feb 2018 12:20:58 +0000 schrieb Bruno P. Kinoshita: >> >> >>> Hello, >> >>> >> >>> If memory serves me well, some time ago we had a discussion around >> >>> sanselan & commons-imaging 1.0. One of the issues with >>> commons-imaging >> >>> 1.0 was the Debug class. >> >>> >> >>> https://issues.apache.org/jira/browse/IMAGING-154 >> >>> >> >>> I finished the pull request, but Gilles raised an important point, >>> about >> >>> discussing other alternatives first. >> >>> >> >>> Initially I am against logging in low level libraries, especially >> >>> commons components. But some time ago I had to debug TIFF issues in >> >>> commons-imaging, and having the dump methods was a tremendous help. >> >>> >> >>> >> >>> The issue is that some imaging algorithms/processing have a lot of >> >>> variables that can be altered. And keeping an eye on all of them in >>> the >> >>> debugger can be quite hard - though not impossible. >> >>> >> >>> So all in all, now I am more confident to proceed without the Debug >> >>> class. But some users could have a hard time investigating possible >> >>> issues in the library without seeing what's going on within the >>> library. >> >>> >> >>> IMO, that could be solved with the logging/dump features... or >>> through a >> >>> better design, especially around exception handling/throwing. The >>> latter >> >>> is my preferred approach. Instead of logging, I prefer - whenever >> >>> possible - that low level libraries throw exceptions and let me >>> handle >> >>> the logging. >> >>> >> >>> >> >>> So, any thoughts? :) I'm +1 to remove the Debug class, and +0 to a >> >>> logging added to commons-imaging. >> >>> >> >>> Bruno --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
