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

New functions for the PubChem PUG-REST API #239

Merged
merged 16 commits into from
May 15, 2020
Merged

Conversation

stitam
Copy link
Contributor

@stitam stitam commented Apr 16, 2020

The PR is related to issues #206 and #216.

I originally created two new functions. pc_pugrest() gave a lot of control over the PUG-REST API and pc_validate() validated all inputs to pc_pugrest(). The aim was to offer functions for more advanced users to assemble their own requests. However, pc_validate became too complex and very difficult to test, and so I decided to just remove both functions. Instead, I created the function get_sid() for issue #216 and will edit get_cid() for issue #206 after @Aariq updated it according to #193 so there is no conflict.

PR task list:

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

@codecov-io
Copy link

codecov-io commented Apr 16, 2020

Codecov Report

Merging #239 into master will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #239   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          19      19           
  Lines        1724    1786   +62     
======================================
- Misses       1724    1786   +62     
Impacted Files Coverage Δ
R/pubchem.R 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73dd2cb...ec60d27. Read the comment docs.

@stitam stitam closed this Apr 17, 2020
@stitam stitam reopened this Apr 18, 2020
@stitam
Copy link
Contributor Author

stitam commented Apr 29, 2020

The formula in #206 didn't work because the query is sometimes asynchronous and returns a listkey which has to be queried in a subsequent request, similarly to ChemSpider. I restructured get_cid() to handle this situation as well. I tried to implement all query routes that use PUG-REST API to return cids. https://pubchemdocs.ncbi.nlm.nih.gov/pug-rest.

@stitam stitam requested a review from Aariq May 14, 2020 11:55
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.

I agree that we shouldn't worry about #216 right now and get this merged. This adds a TON of functionality—awesome contribution. I've only made minor suggestions.

R/pubchem.R Outdated Show resolved Hide resolved
R/pubchem.R Show resolved Hide resolved
}
else {
res <- httr::POST(qurl, user_agent("webchem"),
handle = handle(""))
Copy link
Collaborator

Choose a reason for hiding this comment

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

add user_agent() ?

R/pubchem.R Show resolved Hide resolved
R/pubchem.R Show resolved Hide resolved
@stitam stitam merged commit 786d074 into ropensci:master May 15, 2020
@stitam stitam deleted the i206 branch May 15, 2020 08:33
@stitam stitam added this to the RC2019F milestone Sep 5, 2020
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