search

schrodinger / coordgenlibs

Schrodinger-developed 2D Coordinate Generation
BSD 3-Clause "New" or "Revised" License
42 stars 28 forks source link

SHARED-7356: Reduce the scope of the public interface to coordgen #84

Closed rachelnwalker closed 3 years ago

rachelnwalker commented 3 years ago

Made all fields in the sketcherMinimizer class private, and added appropriate getters, setters, and other methods to interact with CoordgenMinimizer and CoordgenFragmentBuilder. Requires a few updates to CoordGen.h in RDKit.

ricrogz commented 3 years ago

I think we should also bump the SOVERSION (https://cmake.org/cmake/help/latest/prop_tgt/SOVERSION.html) in the CMakeFile, to signal that the API has changed.

rachelnwalker commented 3 years ago

I just made all public sketcherMinimizer methods that are not used outside of sketcherMinimizer private (except for getters/setters). My hope is that we can decide which of those functions should be public and which can stay private

d-b-w commented 3 years ago

@ricrogz - I was thinking that I should bump the soversion when I bump the release. Do you think it would make more sense to do beforehand, instead?

ricrogz commented 3 years ago

@ricrogz - I was thinking that I should bump the soversion when I bump the release. Do you think it would make more sense to do beforehand, instead?

Yes, I think we should bump the soversion as soon as we change the API. Otherwise, if anyone builds master, and attempts to use it as a drop-in replacement of a previous build, they will have problems. We might also bump the version number and add a "dev" suffix at the same time, if that makes more sense.

rachelnwalker commented 3 years ago

Is there a way for test_coordgen to have access to CoordgenFragmentBuilder and CoordgenMacrocycleBuilder without exporting those classes? I get this error even when I include their headers:

Undefined symbols for architecture x86_64:
  "CoordgenFragmentBuilder::orderRingAtoms(sketcherMinimizerRing const*)", referenced from:
      testGetDoubleBondConstraints::test_method() in test_coordgen.cpp.o
  "CoordgenMacrocycleBuilder::getDoubleBondConstraints(std::__1::vector<sketcherMinimizerAtom*, std::__1::allocator<sketcherMinimizerAtom*> >&) const", referenced from:
      testGetDoubleBondConstraints::test_method() in test_coordgen.cpp.o
rachelnwalker commented 3 years ago

I updated these changes to avoid requiring changes in this RDKit header for now.