Skip to content
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

MAINT: improve error with SMILES #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tylerjereddy
Copy link

* The error message when processing conventional SMILES strings
can be confusing, as observed at:
marrink-lab/polyply_1.0#390 (comment)

* For cases like that where there are no polymeric fragments (i.e., no
use of braces), this patch improves the error message (and adds a test
for it) to nudge the user to BigSMILES or CGSmiles formats.
Copy link
Collaborator

@fgrunewald fgrunewald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. Since we only parse CGSmiles at the moment, I'd remove BigSMILES from the error message.

cgsmiles/resolve.py Outdated Show resolved Hide resolved
cgsmiles/tests/test_molecule_resolve.py Outdated Show resolved Hide resolved
* Adjust the error message for usage of unsupported
conventional SMILES strings to only recommend CGSmiles
format, based on reviewer feedback.

* Fixed a bug where `test_syntax_errors` test was not
actually checking the correctness of the emitted
error strings, then fixed any issues that slipped
through because of this.
@tylerjereddy
Copy link
Author

tylerjereddy commented Nov 6, 2024

I pushed a commit that attempts to resolve the first round of review comments.

Also, I noticed that test_syntax_errors wasn't actually verifying that the error message strings matched, so I went ahead and fixed that by using the canonical match=.. syntax, and that now correctly enforces matching error strings (I fixed one small issue that slipped through previously because of this).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants