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!

@Aariq
Copy link
Collaborator

Aariq commented Jan 6, 2025

#295 is basically saying we should return tibbles whenever possible rather than nested lists, if I remember correctly.

@Aariq
Copy link
Collaborator

Aariq commented Jan 6, 2025

I know this is still a WIP, but could you update the documentation to include info on the new from options when you get a chance? That would make it easier to give feedback on this PR.

Comment on lines +88 to +93
"https://pesticidecompendium.bcpc.org/index_rn.html",
"www.bcpcpesticidecompendium.org/index-inchikey.html",
"https://pesticidecompendium.bcpc.org/index_cn.html",
"https://pesticidecompendium.bcpc.org/index-fr.html",
"https://pesticidecompendium.bcpc.org/index-ru.html",
"https://pesticidecompendium.bcpc.org/index-zh.html"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

@AlexisBrgn AlexisBrgn Jan 7, 2025

Choose a reason for hiding this comment

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

I admit I blindly pasted the format from the previous code.
These url actually redirect to http://www.bcpcpesticidecompendium.org.
For instance https://pesticidecompendium.bcpc.org/index_rn.html -[302]-> http://www.bcpcpesticidecompendium.org/index_rn.html
However the "redirection" to a *-frame.html is initiated by http://www.bcpcpesticidecompendium.org/index_{from}.html in javascript

if (top==self) self.location.href="index_rn_frame.html";

and should not happen when the code executes.
I will therefore replace all the URL to the format http://www.bcpcpesticidecompendium.org/index_{from}.html.

@stitam
Copy link
Contributor

stitam commented Jan 6, 2025

#295 is basically saying we should return tibbles whenever possible rather than nested lists, if I remember correctly.

Yes, I also think the issue was about returning tibbles. Mote than 4 years later I still think it's a good idea :) Without looking at the code in the PR I would think tibbles may be out of scope for this PR but if it's already done, great :)

@AlexisBrgn
Copy link
Author

#295 is basically saying we should return tibbles whenever possible rather than nested lists, if I remember correctly.

Yes, I also think the issue was about returning tibbles. Mote than 4 years later I still think it's a good idea :) Without looking at the code in the PR I would think tibbles may be out of scope for this PR but if it's already done, great :)

Looking at the code I think it is out of scope too, but I'm willing to tackle the issue in another PR.

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