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

Update OCL to v2022.10.2 #141

Merged
merged 2 commits into from
Oct 17, 2022
Merged

Update OCL to v2022.10.2 #141

merged 2 commits into from
Oct 17, 2022

Conversation

targos
Copy link
Member

@targos targos commented Oct 16, 2022

  • chore: update dependencies
  • feat: update OCL to v2022.10.2
  • test: update diastereotopicIDs test for new OCL version

Comment on lines 24 to 40
expect(diaIDsH).toMatchInlineSnapshot(`
[
"gOp@DiekjjiJ@qAP_iDCaU@",
"gOp@DiVMjjij@qAP_iDCaU@",
"gOp@DiWMjj\`FHJC}H\`\\Jh",
"gOp@DjWkjj\`FHJC}H\`\\Jh",
"gOp@DfUkjj\`FHJC}H\`\\Jh",
"daD@\`J@DjYvzjjdBbB@_iBTbUP",
"daD@\`J@DjYvzjjdBbJ@_iBTbUP",
"daD@\`N@DjWjZjjdB\`b@_iB\\bUP",
"daD@\`N@DjWjZjjdB\`j@_iB\\bUP",
"daD@\`N@DjWzZjjdBL\`_iB\\bUP",
"daD@\`N@DjWzZjjdBM@_iB\\bUP",
"daD@\`B@LddRK]UUP@RDIDj\`",
"daD@\`F@DjUZzjj\`A~dHrIU@",
]
`);
Copy link
Member Author

Choose a reason for hiding this comment

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

@lpatiny Some ids have changed. Should we do a major release of this library? I don't know exactly what the difference is. Maybe the new ids are not compatible with older versions (how can we know?).

Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand correctly, the changes are an addition of 3 new query feature fields:
image

Copy link
Member

Choose a reason for hiding this comment

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

For me it is quite complex because we store the NMR assignment as a diastereotopicID (in the .nmrium file for example). This means that changing the code force us to have a migration path and it would not be compatible with previous versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

What should I do?

Copy link
Member

Choose a reason for hiding this comment

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

I created an issue:

Actelion/openchemlib#80

I tested the new code and they are not depicted correctly with the old version (current JS version) of openchemlib. It can be tested on https://www.cheminfo.org/?viewURL=https%3A%2F%2Fcouch.cheminfo.org%2Fcheminfo-public%2F1cc9e892242664b1d5a37312bda159ef%2Fview.json&loadversion=true&fillsearch=Display+OCLcode+oclID

But to be sure I would need that you 'escape' the result because I'm not sure there are no problematic backspace and we deal correctly with escape.

Are you able to add here the list of the 'escape' of all the strings ? (the view unescape them)

Copy link
Member Author

Choose a reason for hiding this comment

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

escaped version:

      [
        'gOp@DiekjjiJ@qAP_iDCaU@',
        'gOp@DiVMjjij@qAP_iDCaU@',
        'gOp@DiWMjj%60FHJC%7DH%60%5CJh',
        'gOp@DjWkjj%60FHJC%7DH%60%5CJh',
        'gOp@DfUkjj%60FHJC%7DH%60%5CJh',
        'daD@%60J@DjYvzjjdBbB@_iBTbUP',
        'daD@%60J@DjYvzjjdBbJ@_iBTbUP',
        'daD@%60N@DjWjZjjdB%60b@_iB%5CbUP',
        'daD@%60N@DjWjZjjdB%60j@_iB%5CbUP',
        'daD@%60N@DjWzZjjdBL%60_iB%5CbUP',
        'daD@%60N@DjWzZjjdBM@_iB%5CbUP',
        'daD@%60B@LddRK%5DUUP@%7FRDIDj%60',
        'daD@%60F@DjUZzjj%60A%7EdHrIU@'
      ]

@targos targos merged commit 64b7d9a into main Oct 17, 2022
@targos targos deleted the update-ocl branch October 17, 2022 15:05
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.

None yet

2 participants