methods to manipulate atom types in ReactionManipulator#883
methods to manipulate atom types in ReactionManipulator#883johnmay merged 2 commits intocdk:masterfrom
Conversation
uli-f
commented
Aug 26, 2022
- added methods to perceive and unset the atom type configurations in ReactionManipulator
- fixed a javadoc errors wrt content in AtomContainerManipulator#percieveAtomTypesAndConfigureUnsetProperties
… ReactionManipulator - fixed a javadoc errors wrt content in AtomContainerManipulator#percieveAtomTypesAndConfigureUnsetProperties
egonw
left a comment
There was a problem hiding this comment.
Please add test methods for all new methods. And thanx for the extensive documentation!
I decided against adding tests as the methods merely combine two (already tested) method calls. There is not logic in those methods other than a for loop. I am happy to add tests if that is what you prefer (e.g., to achieve good test coverage numbers). |
Understood. But... we write unit tests as much as now as for the future. The implementation is now simple, but in the future may not. I found it much more beneficial for maintainability if all assumption are explicit. It fixed a lot of code in the 2003-2008 period, even the silliest unit tests. |
Okay, I see your point. |
- added null pointer checks for perceiveAtomTypesAndConfigureAtoms, perceiveAtomTypesAndConfigureUnsetProperties, and clearAtomConfigurations
|
Added tests now. |
|
Looks fine, personally I don't think we use wildcard imports anywhere else mainly for convention. In the restricted case of Although that would be pretty silly :-). |