New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use interfaces (and builders) instead of implementations #187
Conversation
MoleculeFactory was replaced with TestMoleculeFactory.... I couldn't remove MoleculeFactory so stuck it in misc. |
[MoleculeFactory]... mmm, too bad there was not a @deprecated :/ |
misc is effectively don't use this... :p like the IUPAC parser... |
API change on MoleculeFactory... not sure how widely used it is by outside the main library but we should have a stable API. |
"misc is effectively don't use this" --> huh? I'd say log4j, diff at least are very much meant to be used! under the right conditions... and extra certainly has important functionality, and so does control (for Bioclipse, but also JChemPaint, though they forked that) |
"API change on MoleculeFactory" -> to me, not depending on interfaces but on implementations is a bug. |
BTW, while I disagree about using 'misc'... for MoleculeFactory I do agree... that should not really be used. As far as I know, it only really existed to aid the tests... it seems to me that when TestMoleculeFactory got introduced, not all tests started using it, because these patches actually update many test-* classes... So, shall I just remove the MoleculeFactory patch then? |
Need too look more into it, but yes essentially all test usages should be TestMoleculeFactory... I think there was some usage outside of tests though (but only in misc)? Disagree on the interfaces comment by the way, it's not black and white. If there's only one implementation it's perfectly reasonable to use the concrete instances. With the molecules it's very confusing to new comers the whole data/silent usage as the wrong one is the default :-). |
My mistake on misc, I thought that was the module... I meant 'extra'. It's not that it's not useful it's that it doesn't fit and hence may be out of scope. |
["the wrong one is the default"]... well, for long JChemPaint was a key users which critically depended on this functionality... the whole idea of interfaces is to actually not being forced to use this "wrong one"! That's why I'm trying to make everything not depending on them... actually, the cdk-silent is "wrong" too... I still think the implementation can yet be a factor faster... why not make an implementation that uses a connection matrix internally, and not use List at all? |
The history of the "extra" module is actually that *everything" is in extra... from that starting point, I started putting stuff in other modules :) There is still stuff in "extra" that should probably go somewhere else... |
I'll make a patch that moves all usage of MoleculeFactory over to TestMoleculeFactory... otherwise API changes need addressing. |
Sounds good. |
(I may redo some of this at some later point, but likely just from scratch...) |
No description provided.