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

get_*() consistency #242

Merged
merged 37 commits into from
Apr 28, 2020
Merged

get_*() consistency #242

merged 37 commits into from
Apr 28, 2020

Conversation

Aariq
Copy link
Collaborator

@Aariq Aariq commented Apr 22, 2020

Pull Request

This addresses #193 and implements a consistent API and output (tibble) format for all get_*() functions.
It also adds a new (non-exported) utility function, matcher that maybe should superseed chooser eventually. This should be able to handle the match argument of most functions.

PR task list:

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

Aariq added 29 commits April 14, 2020 22:06
…bit. Now works with the matcher utility and outputs a tibble.
Merge branch 'master' into git-consistency

# Conflicts:
#	NAMESPACE
#	tests/testthat/test-chemspider.R
Merge branch 'master' into get_consistency

# Conflicts:
#	DESCRIPTION
#	NAMESPACE
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! I am very happy that this PR implements the conclusions of our earlier discussion, the tibble upgrade and matcher() are great! One thing I noticed, we seem to be increasing the number of dependencies (I also added data.tree in another PR), maybe this is something we should discuss in more detail in another issue.

R/chemspider.R Outdated Show resolved Hide resolved
R/chemspider.R Show resolved Hide resolved
R/chemspider.R Outdated Show resolved Hide resolved
tests/testthat/test-chemspider.R Show resolved Hide resolved
tests/testthat/test-wikidata.R Show resolved Hide resolved
@Aariq Aariq linked an issue Apr 23, 2020 that may be closed by this pull request
@codecov-io
Copy link

codecov-io commented Apr 26, 2020

Codecov Report

Merging #242 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #242   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          19      19           
  Lines        1750    1750           
======================================
  Misses       1750    1750           

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 9a83f0a...9a83f0a. Read the comment docs.

DESCRIPTION Outdated Show resolved Hide resolved
R/pubchem.R Outdated Show resolved Hide resolved
R/etox.R Outdated Show resolved Hide resolved
@Aariq Aariq linked an issue Apr 27, 2020 that may be closed by this pull request
@Aariq Aariq requested a review from stitam April 27, 2020 15:31
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.

I think this PR is ready to be merged. Thanks @Aariq!!

@stitam stitam merged commit 73dd2cb into ropensci:master Apr 28, 2020
@Aariq Aariq deleted the get_consistency branch April 28, 2020 14:01
@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.

examples in etox_*() don't run. get_*() consistency discussion
3 participants