-
Notifications
You must be signed in to change notification settings - Fork 42
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
base: master
Are you sure you want to change the base?
Feature bcpc extend features #420
Conversation
Added name_fr, name_zh, name_ru as scraped fields from page
…age, so +3 length for fluazinam), and new `bcpc_query` from arguments.
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? |
Thanks for the heads up I'll dig into it as soon as I manage to free up some time to contribute.
Yes it is still very rough, the basic features work! |
#295 is basically saying we should return tibbles whenever possible rather than nested lists, if I remember correctly. |
I know this is still a WIP, but could you update the documentation to include info on the new |
"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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These URLs all appear to re-direct. E.g. http://www.bcpcpesticidecompendium.org/index-inchikey-frame.html, http://www.bcpcpesticidecompendium.org/index-zh-frame.html
There was a problem hiding this comment.
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
.
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. |
Pull Request
Brief description of the PR
Extended features from bcpc.R :
bcpc_query
now returns localesThis 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:
devtools::document()