Add the option to locate a IChemObject builder using reflection. Enfo…#1140
Add the option to locate a IChemObject builder using reflection. Enfo…#1140
Conversation
…rce the RDfile reader has one.
|
Looks good. Same as for the other PR, for 2.10.x or 2.11? |
uli-f
left a comment
There was a problem hiding this comment.
I appreciate the concept of the builder cache. The reason it's required for RdfileReader might be an oversight on my end (see comment below).
Happy to carry out the suggested changes myself, whatever suits you better.
| @@ -137,7 +137,7 @@ public RdfileReader(InputStream in, IChemObjectBuilder chemObjectBuilder, boolea | |||
| * @param reader the Reader providing the RDfile data | |||
| */ | |||
| public RdfileReader(Reader reader) { | |||
There was a problem hiding this comment.
This method signature looks like an oversight on my end, my apologies. Looking at the javadocs it states that it takes an IChemObjectBuilder; moreover, both constructors that accept an InputStream also require an IChemObjectBuilder.
Suggestion: I would mark this constructor @deprecated and add a new one:
public RdfileReader(Reader reader, IChemObjectBuilder chemObjectBuilder) {
this(reader, chemObjectBuilder, true);
}
| import org.openscience.cdk.tools.LoggingToolFactory; | ||
|
|
||
| import java.lang.reflect.InvocationTargetException; | ||
| import java.lang.reflect.Method; |
|
|
I think we still have the IChemObjectBuilder cache as Egon and I had considered this previously. @egonw good to merge if you like. |
…rce the RDfile reader has one.
Different Java version's didn't like a piece of code the the RDfile reader constructor - I spotted it when I wrote the sample code for the release and accidentally committed it here: f351dd0#diff-c8b0d51b33ab2fde91ce878d3b9887e6063201b65be2fa98b6a842643a4b4fceL138-R140.
I have added the option for the IChemObjectBuilder to look one up via reflection, @egonw and I have talked about this previously.