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

Properties with new lines can create invalid SDFiles #5827

Closed
bp-kelley opened this issue Dec 1, 2022 · 2 comments
Closed

Properties with new lines can create invalid SDFiles #5827

bp-kelley opened this issue Dec 1, 2022 · 2 comments
Assignees
Labels
Milestone

Comments

@bp-kelley
Copy link
Contributor

bp-kelley commented Dec 1, 2022

Describe the bug
Adding a string property with multiple new lines generates invalid SD blocks.

To Reproduce

>>> m.SetProp("foo", "docked\n\ndocked")
>>> out = Chem.SDWriter(sys.stdout)
>>> out.write(m)
>>> out.close()

     RDKit          2D

  1  0  0  0  0  0  0  0  0  0999 V2000
    0.0000    0.0000    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0
M  END
>  <foo>  (1) 
docked

docked

$$$$

Expected behavior
RDKit should either reject writing these properties or strip the new lines. It shouldn't write an invalid file.

@bp-kelley bp-kelley added the bug label Dec 1, 2022
@greglandrum
Copy link
Member

It's easy enough to fix this. The question is what the right behavior is:

  1. Report error and fail to write the molecule
  2. Report warning and skip the property for that molecule
  3. Report warning and just write the first line
  4. Report warning and somehow modify the property so that it doesn't contain the \n

I would not do either 1 or 4, but I think either 2 or 3 would be ok. I guess I have a slight preference for 2.

@greglandrum greglandrum added this to the 2022_09_4 milestone Dec 15, 2022
@greglandrum greglandrum self-assigned this Dec 15, 2022
greglandrum added a commit to greglandrum/rdkit that referenced this issue Dec 15, 2022
@greglandrum
Copy link
Member

Ok, I implemented solution 2 in the fix in #5873

greglandrum added a commit that referenced this issue Jan 16, 2023
* fixes #5827

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

No branches or pull requests

2 participants