OB API/ABI changes

>> 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

> 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 <[hidden email]> 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
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/openbabel-devel
>

> 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 <[hidden email]> 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
>> [hidden email]
>> https://lists.sourceforge.net/lists/listinfo/openbabel-devel
>>
>

> 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 <[hidden email]>
>> 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
>> [hidden email]
>> 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: [hidden email]
>     http://www.scripps.edu/~forli/

> 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 <[hidden email]> 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 <[hidden email]>
>>> 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
>>> [hidden email]
>>> 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: [hidden email]
>>     http://www.scripps.edu/~forli/

> 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 <[hidden email]> 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 <[hidden email]>
>>> 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
>>> [hidden email]
>>> 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: [hidden email]
>>     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
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/openbabel-devel
>

> On 02/06/16 08:33, Noel O'Boyle 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.
>>
> How about combining that with my proposal?
>
>> Regards,
>> - Noel
>>
>> On 27 May 2016 at 19:47, Stefano Forli <[hidden email]> 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 <[hidden email]>
>>>> 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
>>>> [hidden email]
>>>> 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: [hidden email]
>>>     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
>> [hidden email]
>> https://lists.sourceforge.net/lists/listinfo/openbabel-devel
>>
>
>
> --
> David van der Spoel, Ph.D., Professor of Biology
> Dept. of Cell & Molec. Biol., Uppsala University.
> Box 596, 75124 Uppsala, Sweden. Phone:  +46184714205.
> [hidden email]    http://folding.bmc.uu.se
>
> ------------------------------------------------------------------------------
> 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
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/openbabel-devel

>> SMARTS pattern is that you have the match of each atom to each virtual
>> atom of the pattern. For a function of OBAtom, it's just boolean true
>> or false.
>
> Moreover, there's already OBAtom::MatchesSMARTS()
> http://openbabel.org/dev-api/classOpenBabel_1_1OBAtom.shtml#abfdefac42d3c895920f1f715f0d710a5
>
> Some of these "one-off" functions are certainly more efficient, and I do like David's enum-based suggestion.
>
> My feeling is that we should decide fairly quickly on any API changes so that we can issue 2.4 soon.
>
> Thanks,
> -Geoff

>> SMARTS pattern is that you have the match of each atom to each virtual
>> atom of the pattern. For a function of OBAtom, it's just boolean true
>> or false.
>
> Moreover, there's already OBAtom::MatchesSMARTS()
> http://openbabel.org/dev-api/classOpenBabel_1_1OBAtom.shtml#abfdefac42d3c895920f1f715f0d710a5
>
> Some of these "one-off" functions are certainly more efficient, and I do like David's enum-based suggestion.
>
> My feeling is that we should decide fairly quickly on any API changes so that we can issue 2.4 soon.
>
> Thanks,
> -Geoff