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

Implementing PubChem PUG-View web service #230

Merged
merged 18 commits into from May 4, 2020
Merged

Conversation

stitam
Copy link
Contributor

@stitam stitam commented Mar 26, 2020

The Pubchem PUG-View web service allows the programmer to access information from the PubChem content pages (e.g. https://pubchem.ncbi.nlm.nih.gov/compound/176). The proposed function which implements this functionality is pc_sect() as it accesses a section of the content pages (I struggled with naming this function so I am open to suggestions). An example:

pc_sect(c(176, 311), "pka")

This returns a tibble of pKa a values. Under the hood the function uses pc_page() to retrieve a list of content pages and pc_extract() to extract information from these content pages and assemble the tibble.

Some issues I feel we should discuss:

  • pc_page() uses webchem_submit() (now renamed to submit_request()) to submit POST requests in a standardised manner. This function is an attempt to standardise all http requests in webchem. Do you think this function would help? Edit: removed the function, let's discuss this as a separate issue.
  • pc_page() and pc_extract() use the data.tree package, which is a new dependency. It would be great not to depend on another package, but I couldn't navigate the content pages otherwise. Can you suggest an alternative?

PR task list:

  • Update NEWS
  • Add tests (if appropriate)
  • Update documentation with devtools::document()
  • Check package passed

@stitam stitam closed this Apr 17, 2020
@stitam stitam reopened this Apr 18, 2020
R/pubchem.R Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/pubchem.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
@stitam stitam requested a review from Aariq April 30, 2020 13:43
Copy link
Collaborator

@Aariq Aariq left a comment

Choose a reason for hiding this comment

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

This function works exactly how I'd expect it to from your description. I think it's a great addition to the package. I have a few documentation requests and some other code suggestions that you can take or leave.

R/pubchem.R Outdated Show resolved Hide resolved
R/pubchem.R Outdated Show resolved Hide resolved
R/ping.R Outdated Show resolved Hide resolved
DESCRIPTION Show resolved Hide resolved
R/pubchem.R Show resolved Hide resolved
@stitam
Copy link
Contributor Author

stitam commented May 4, 2020

I updated the PR based on @Aariq's comments. The package is still not building, I think this has to do with AppVeyor being unable to check the examples because some of the webservices are down.

@Aariq Aariq merged commit 21e1cf7 into ropensci:master May 4, 2020
@stitam stitam added this to the RC2019F milestone Sep 5, 2020
@stitam stitam deleted the pubchem branch October 26, 2020 21:17
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

3 participants