New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow "*" atoms to be created. #269
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is a good idea... it's definitely an API change...
@@ -536,6 +536,10 @@ private static boolean parseAtomSymbol(IAtom atom, String str) { | |||
atom.setAtomicNumber(0); | |||
atom.setSymbol("R"); | |||
return true; | |||
} else if ("*".equals(str)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what we have the PseudoAtom for... this changes the API...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't an API change (API change = signature change this would be functionality change)... it actually used to be possible: c30256c.
However I realised this isn't the correct fix - I'll send that though now.
Related #246 |
Second commit was what actually needed fixing... however I still think the first one is correct. |
Well, it makes PseudoAtom less important... so, yes, a functional change. As long as it is a conscious choice, I had my chance to express my mild concern. Downstream code/algorithms will now have to take into account that an IAtom implementation that is not an IPseudoAtom can still not have a valid atom number... I won't block :) |
FYI... "Downstream code/algorithms will now have to take into account that an IAtom implementation that is not an IPseudoAtom" see the tests on #246. I think that assumption was already made. |
(sorry... I'm getting old and way too busy...) |
No description provided.