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

search for RIs by name, inchi, or inchikey in addition to CAS #232

Merged
merged 27 commits into from Apr 16, 2020

Conversation

Aariq
Copy link
Collaborator

@Aariq Aariq commented Mar 28, 2020

This PR mainly addresses #219. It adds the ability to search for retention indices from NIST using InChI, InChIkey, or compound name.

Bugs fixed:

  • I discovered and fixed a bug where compounds that only had one entry would produce malformed output.

Dependencies added:

  • polite. I switched nist_ri() to use if for web scraping and I quite like using it, but I'm happy to remove this dependency if you don't like it.
  • rlang. I did a few things with rlang that I do not know how to do otherwise.
  • tibble. I switched the output to a tibble rather than a data.frame.

I'm not sure why so much of the README changed. It looks like maybe it just didn't get re-built with some previous PRs.

PR task list:

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

@codecov-io
Copy link

codecov-io commented Mar 28, 2020

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #232   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          19      19           
  Lines        1611    1672   +61     
======================================
- Misses       1611    1672   +61     
Impacted Files Coverage Δ
R/nist.R 0.00% <0.00%> (ø)
R/chemspider.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 434cd60...b050864. Read the comment docs.

DESCRIPTION Show resolved Hide resolved
@Aariq
Copy link
Collaborator Author

Aariq commented Mar 29, 2020

You know, I just realized I changed the scrape delay, but never tested this on large numbers of queries. Let me do some testing and see if I don't get my IP banned before merging this PR

@Aariq
Copy link
Collaborator Author

Aariq commented Mar 29, 2020

Ok, fixed it up so it works with NAs better. New tests are all passing locally.

Copy link
Contributor

@stitam stitam left a comment

Choose a reason for hiding this comment

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

Thanks @Aariq, this is great! Most of my comments were on the same topic, which is coding stye when connecting to other packages. I am not sure which is the best way to do this, but I see that we are not consistent within the package. @import, @importFrom, package::function(), what do you think?

I know I added v0.6.0 to NEWS.md but maybe it would be better to return to incrementing the version number with each PR, 0.5.1, 0.5.2, etc? http://r-pkgs.had.co.nz/release.html.

NEWS Outdated Show resolved Hide resolved
R/nist.R Outdated Show resolved Hide resolved
R/nist.R Outdated Show resolved Hide resolved
R/nist.R Outdated Show resolved Hide resolved
R/nist.R Outdated Show resolved Hide resolved
R/nist.R Outdated Show resolved Hide resolved
R/nist.R Outdated Show resolved Hide resolved
R/nist.R Outdated Show resolved Hide resolved
R/nist.R Outdated Show resolved Hide resolved
tests/testthat/test-nist.R Outdated Show resolved Hide resolved
@stitam stitam mentioned this pull request Apr 7, 2020
@stitam
Copy link
Contributor

stitam commented Apr 16, 2020

I ran R CMD check locally and it succeeded.

@stitam stitam merged commit 0d0df09 into ropensci:master Apr 16, 2020
@Aariq Aariq deleted the dev_nist branch April 16, 2020 15:40
@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.

None yet

3 participants