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

Dev #225

Closed
wants to merge 4 commits into from
Closed

Dev #225

wants to merge 4 commits into from

Conversation

anjuraj15
Copy link

Hi Rene,

We updated our existing records with additional information and deprecated some of the records due to wrong annotation.

Thanks,
Anjana

@schymane
Copy link
Member

See also MassBank/RMassBank#331
Unfortunately the commit fixing the SMILES got merged into the big set ...

Pls let us know if you need any more info @meier-rene

@meier-rene
Copy link
Collaborator

meier-rene commented May 22, 2023

Thank you for your contribution. The SMILES issue is known and I gave some explanations in MassBank/RMassBank#331. For these particular 5 compounds the SMILES without cis/trans information are actually a better choice.

There was one minor issue with your contribution: There were modifications to records which were DEPRECATED before. DEPRECATED records should not be touched after deprecation. LCSB/MSBNK-LCSB-LU08010* and LCSB/MSBNK-LCSB-LU09280* are the affected files. I need to think about a technical solution to prevent that in the future.

I took these changes out of the commit and committed again. The remaining changes were manually merged by me. Thats why I will close this PR but all your changes (except the ones on the deprecated records) are merged into the dev branch.

@meier-rene
Copy link
Collaborator

Thank you one more time. All merged.

@meier-rene meier-rene closed this May 22, 2023
@meier-rene
Copy link
Collaborator

One more thing: Because I messed around with your commits its probably better if you reset your repo to remote the branch.
Something like

git fetch origin
git reset --hard origin/dev

or fork again should do the trick.

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.

3 participants