Sorry - I've just reread your reply again. In the case of the OBBond, it sounds like you think it would be useful for these to be in the public API. Maybe I should look through these functions a bit first.
- Noel On 2 June 2016 at 07:33, Noel O'Boyle <baoille...@gmail.com> wrote: > Hi Stefano, > > Short functions are good - I should really be better at this - but > there is no need to add all of your functions to the public API. From > what you say, it sounds like this was unintentional. In that case, is > it okay if I change things to remove these utility functions from the > public API? This will involve moving them from member functions of > OBAtom/OBBond to static functions, e.g. from OBAtom::IsXXXX() to > static OBAtom_IsXXXX(OBAtom*). Functionally everything should be the > same. > > Regards, > - Noel > > On 27 May 2016 at 19:47, Stefano Forli <fo...@scripps.edu> wrote: >> I welcome this discussion because it addresses some of the issues I had when >> starting contributing to the code. >> >> I'm partially responsible of several convenience functions (IsSulfoneOxygen, >> to name one :), and I would be happy to have a guideline to follow to deal >> with the issue. >> >> The rationale is: I've been schooled by experienced programmers to keep >> functions as simple as possible in order to limit bugs (like that rock band >> in which they paint their faces black and white). >> >> Prior to my patch, IsHbondAcceptor was just a handful of lines, and adding >> features made it grow to several 'pages' of source. Adding convenience >> functions was an attempt to improve code clarity and reduce chances of bugs. >> Although, I agree with Noel in terms of not creating ad-hoc functions which >> are used only once. If there's a way to group them together to keep them >> isolated from the main code (or not public?), I'm fine with it. >> I'm a big fan of generalizations over special cases. >> >> A similar thing is happening for the OBBond::IsEster function, but in this >> case I don't know how to deal with new features. >> I've tried fixing a possible bug and extend the functionalities, so the code >> went from 29 to 296 lines (including some documentation/comments). >> In order to cover all esters accordingly to IUPAC (and Wikipedia, of >> course), I ended up in writing extra functions that are called by IsEster >> itself, but can still be called independently: >> >> IsEster: >> ->IsPhosphateEster >> ->IsCarboxyEster >> ->IsThioEster >> >> As a consequence, the ABI would change too, and IsEster now could accept >> several extra boolean flags to deal with these extra features (before it >> accepted none). >> >> I think these extra functions could be considered convenience functions. In >> principle, the whole code could be in a single, larger function, which will >> have one well-defined behavior. >> >> Although, I think this would imply the inability to customize the behavior >> of IsEster, and limit the access to new features. >> >> In a possible scenario, a user could call IsEster(), and then write its own >> code to get discriminate between the different ester types. Since the >> function already did the job, why not make it available directly? >> >> I'm aware that this is a complex reply to what was a simple and well-posed >> point, but I would appreciate to have such aspects clarified to prevent bad >> coding practices to sneak in the project. >> >> It would be great if eventually such guidelines would land in a nice and >> small "Guidelines" page for OB developers and contributors. >> >> Thanks! >> >> S >> >> >> >> >> On 05/27/2016 08:24 AM, Noel O'Boyle wrote: >>> >>> I was just going to make the point that I'm not too keen on the >>> majority of the additions to OBAtom and OBMol, which appear to be >>> convenience functions. Consider IsSulfoneOxygen. There are a lot of >>> elements out there and a lot of functional groups; we should either >>> add them all or add none. If in the end, this function is absolutely >>> necessary, surely such convenience functions could live elsewhere >>> rather than being in the central classes of the library. >>> >>> I blame IsHydrogen() - it's a magnet for further "Is"s :-) >>> >>> - Noel >>> >>> On 23 May 2016 at 19:06, Geoffrey Hutchison <geoff.hutchi...@gmail.com> >>> wrote: >>>>> >>>>> >>>>> http://baoilleach.webfactional.com/site_media/ob_api_check/compat_report_21052016.html >>>>> It makes a few mistakes here and there but is accurate in general. >>>> >>>> >>>> It's a bit weird about adding optional parameters, e.g. >>>> bool Build(OBMol &mol); >>>> becomes >>>> bool Build(OBMol &mol, bool stereoWarnings = true); >>>> >>>> This isn't an API issue, since a program build to the first will still >>>> recompile. This is an ABI problem, but we've never been too concerned about >>>> that from 2.x to 2.y. >>>> >>>>> I have a few comments to make regarding some of the changes, but will do >>>>> that separately. >>>> >>>> >>>> I'm obviously interested in the feedback. It's also useful to make sure >>>> that source documentation was added. >>>> >>>> Thanks, >>>> -Geoff >>> >>> >>> >>> ------------------------------------------------------------------------------ >>> What NetFlow Analyzer can do for you? Monitors network bandwidth and >>> traffic >>> patterns at an interface-level. Reveals which users, apps, and protocols >>> are >>> consuming the most bandwidth. Provides multi-vendor support for NetFlow, >>> J-Flow, sFlow and other flows. Make informed decisions using capacity >>> planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e >>> _______________________________________________ >>> OpenBabel-Devel mailing list >>> OpenBabel-Devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/openbabel-devel >>> >> >> -- >> >> Stefano Forli, PhD >> >> Assistant Professor of Integrative >> Structural and Computational Biology, >> Molecular Graphics Laboratory >> >> Dept. of Integrative Structural >> and Computational Biology, MB-112A >> The Scripps Research Institute >> 10550 North Torrey Pines Road >> La Jolla, CA 92037-1000, USA. >> >> tel: +1 (858)784-2055 >> fax: +1 (858)784-2860 >> email: fo...@scripps.edu >> http://www.scripps.edu/~forli/ ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e _______________________________________________ OpenBabel-Devel mailing list OpenBabel-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-devel
