Hi David,
I'd love to have the time to do everything perfectly. Yes, we should have a
deprecation phase, with compiler warnings, and messages generated by the
bindings. But eventually, the changes will still break the build. What we
do need are migration docs at the least.
Regards,
- Noel
On 12 January 2018 at 14:43, Koes, David <dk...@pitt.edu> wrote:
> Hi Noel,
>
> I’m mostly just grumpy because these changes broke my build. Have you
> considered going through a deprecated phase?
>
> From your email, I thought you were saying you added an IsElement method
> to OBAtom, but this doesn’t seem to be the case. Something like:
> OBAtom.IsElement(OBElement::Hydrogen)
> is quite readable and it’s much more general than a specific helper
> function. There’s also no need worry about subtle bugs incurred from
> incorrectly accounting for operator precedence (e.g., the == in
> GetAtomicNum() == 1).
>
> You could even shorten the name to “Is”:
> atom->Is(OBElement::Hydrogen)
>
> David Koes
>
> Assistant Professor
> Computational & Systems Biology
> University of Pittsburgh
>
>
> On Jan 12, 2018, at 9:01 AM, Noel O'Boyle <baoille...@gmail.com> wrote:
>
> Hi David,
>
> We don't break backwards compatability lightly. The last time was 12 years
> ago. In that time, there has been some accumulation of cruft, and in
> addition there are some necessary changes in order to fix underlying
> problems.
>
> Regarding the specific functions you mention, the IsElement() functions
> are, as you say, convenience functions, some of which date back to the
> original Stahl/Walters Babel. While GetAtomicNumber() == 1 is less readable
> and error prone as you say, I have added the elements as enums,
> OBElement::Hydrogen, to mitigate this. Furthermore, while the IsElement()
> methods only covered a small number of elements, the ones where people are
> most likely to know the atomic number (where's IsFluorine() for instance),
> the OBElement namespace covers them all.
>
> So why have I removed them? They are convenience functions trivially
> implementable in terms of the remaining API. There is nothing to prevent a
> user implementing the function themselves as a macro or whatever. From the
> port of Open Babel to remove these function, I can give examples of where
> the use of these functions results in inefficient code. Obviously there are
> two function lookups instead of one. But also, I have seen constructions
> like "if atom->IsHydrogen then this; else if atom->IsCarbon then this; and
> so on. In other words, instead of getting the atomic number once and
> switching on it, the maximum possible amount of work is done.
>
> Regarding IsSingle(), the situation is more clear cut. This is a function
> which appears to be (and is documented to be) synonymous with
> GetBondOrder() == 1. Aha, but it is not, and only by looking at the source
> code would you have realised the difference. It was a surprise to me, and I
> think it is used synonymous with GetBondOrder==1 throughout the
> codebase...but there's no way to tell what the caller intended. All we can
> do is prevent nasty surprises for users in the future. This change was made
> in combination with an overhaul of the treatment of kekulization and
> aromaticity last year.
>
> Regards,
> - Noel
>
>
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
OpenBabel-Devel mailing list
OpenBabel-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openbabel-devel
