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

Feature bcpc extend features #420

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

AlexisBrgn
Copy link

@AlexisBrgn AlexisBrgn commented Nov 7, 2024

Pull Request

Brief description of the PR

Extended features from bcpc.R :

  • Added locales to search from : Chinese, French, Russian
  • Added identifier to search from : inchikey
  • Refactored to easily add locales and identifier to search from.
  • bcpc_query now returns locales

This PR is unsolicited but it was a shame the exposed functions didn't allow to search in other languages when the info was right there. I don't like refactoring other people's code but the existing codebase would've made extending features a bit messy.
Maybe this is the occasion to address #295? I'm not sure I understand it clearly tho.

PR task list:

  • Update NEWS
  • Add tests (if appropriate)
  • Unit test extracted functions from the refactoring?
  • Update documentation with devtools::document()
  • Check package passed
  • test-bcpc.R passed

@AlexisBrgn AlexisBrgn marked this pull request as draft November 7, 2024 16:28
@stitam
Copy link
Contributor

stitam commented Dec 23, 2024

Thanks @AlexisBrgn for opening this PR. Some of the webservices changed which broke a few tests we already had. I fixed these, can you please rebase on the current master? After rebase, please also rerun devtools::document(), this will remove man/webchem.Rd.

Is this PR still work in progress?

@AlexisBrgn
Copy link
Author

Thanks for the heads up I'll dig into it as soon as I manage to free up some time to contribute.

Is this PR still work in progress?

Yes it is still very rough, the basic features work!

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