Hi Adrian, Adrian Cumiskey a écrit : > Vincent Hennebert wrote: >> >> Yes, the patch doesn't seem to break anything. We could even go a bit >> further: the cfg parameter is no longer used in the >> FOUserAgent.configure() method, it might be removed. Also, would the >> FopFactory.getUserConfig() method not be public, I would even remove it, >> and move the logic that is in FOUserAgent.getUserRendererConfig into >> FopFactory. Is that getUserConfig() method supposed to have any utility >> to embedding applications? I would answer no, but what do embedding >> specialists think about it? >> >> Vincent >> > > I had considered all these points when developing the patch, and I whole > heartedly agree with them, they are all good ones :-).. but I wanted to > try and be as minimal as possible in my refactoring - at the end of the > day this should just be a fairly simple, straight forward patch after > all. Making these sort of changes makes committer checking more > difficult and time consuming. If you are happy to review my patch file > with the above changes I am *more than happy* to make them :-).
Don't be afraid of refactoring, if it brings significant simplifications. To help the reviewing work, I suggest you to do the following: - don't do unrelated changes in the same patch. Just address one problem at a time; a big change addressing one problem will be simpler to track than several unrelated little changes; - give a detailed changelog along with your patch: that will allow us to check your proposed changes are ok instead of inferring them; - if you move files, tell us, so that we can do svn move instead of deleting the file from the old location and creating a new one, as would be the case by blindly applying the patch. That will allow to keep the history of changes. > Going further along this train of thought.. IMO, in an ideal world all > this config stuff should be abstracted in a separate class such as > FopConfig - so code dependencies on the avalon Configurable interface > would be minimised and configuration would be centrally managed. Interesting. I'm not a specialist of the config area, so I hope someone who has more insight in this area will be able to give comments. If you want to give it a try, please do. Anything that goes towards simplifying the code base is a good thing, IMO. Thanks, Vincent
