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

rgeospatialquality #28

Closed
jotegui opened this Issue Mar 11, 2016 · 15 comments

Comments

Projects
None yet
3 participants
@jotegui

jotegui commented Mar 11, 2016

    1. What does this package do? (explain in 50 words or less)
      It allows users to assess the geospatial quality of biodiversity data by providing native access to the methods of the Geospatial Quality API (http://bit.ly/gqapi_doc_gh)
    1. Paste the full DESCRIPTION file inside a code block below.
Package: rgeospatialquality
Type: Package
Title: R Wrapper for the Geospatial Data Quality REST API
Version: 0.2.2
Date: 2016-02-22
Authors@R: person("Javier", "Otegui", email="javier.otegui@gmail.com",
    role=c("aut", "cre"))
Description: This package provides native wrappers for the functions available
    via de spatial quality REST API. More information on the API can be found
    here: http://bit.ly/bioinformatics_btw057.
License: file LICENSE
LazyData: TRUE
Imports:
    httr (>= 1.0.0),
    jsonlite (>= 0.9.19),
    rgbif (>= 0.9.2)
RoxygenNote: 5.0.1
Suggests: knitr,
    roxygen2,
    rmarkdown,
    testthat
VignetteBuilder: knitr
URL: https://github.com/jotegui/rgeospatialquality
BugReports: https://github.com/jotegui/rgeospatialquality/issues
    1. URL for the package (the development repository, not a stylized html page)
      https://github.com/jotegui/rgeospatialquality
    1. What data source(s) does it work with (if applicable)?
      None
    1. Who is the target audience?
      Biodiversity community: collection managers, researchers
    1. Are there other R packages that accomplish the same thing? If so, what is different about yours?
      There are no other packages, as far as I know
    1. Check the box next to each policy below, confirming that you agree. These are mandatory.
    • This package does not violate the Terms of Service of any service it interacts with.
    • The repository has continuous integration with Travis CI and/or another service
    • The package contains a vignette
    • The package contains a reasonably complete README with devtools install instructions
    • The package contains unit tests
    • The package only exports functions to the NAMESPACE that are intended for end users
    1. Do you agree to follow the rOpenSci packaging guidelines? These aren't mandatory, but we strongly suggest you follow them. If you disagree with anything, please explain.
    • Are there any package dependencies not on CRAN? No
    • Do you intend for this package to go on CRAN? Yes
    • Does the package have a CRAN accepted license? Yes
    • Did devtools::check() produce any errors or warnings? If so paste them below. No error or warning
    1. Please add explanations below for any exceptions to the above:
    1. If this is a resubmission following rejection, please explain the change in circumstances.

@sckott sckott added the package label Mar 11, 2016

@sckott

This comment has been minimized.

Show comment
Hide comment
@sckott

sckott Mar 11, 2016

Member

Reviewers: @sckott

Member

sckott commented Mar 11, 2016

Reviewers: @sckott

@sckott

This comment has been minimized.

Show comment
Hide comment
@sckott

sckott Mar 24, 2016

Member

Nice package! Overall, this is great! Some suggestions for improvements detailed below.

DESCRIPTION file

  • Title field: you have R Wrapper for the Geospatial Data Quality REST API. Just change to Wrapper for the Geospatial Data Quality REST API, as they sometimes complain about the use of R in the title and description, since they say it's obvious it's for R since its on CRAN.
  • Description field: you have This package provides native wrappers for the functions available via de spatial quality REST API. More information on the API can be found here: http://bit.ly/bioinformatics_btw057.. Change de to the

Installation

Works on all platforms, windows and linux installs pass.

travis file

I suggest removing

r_github_packages:
  - jimhester/covr

and put covr in Suggests in DESCRIPTION file

license

Any reason you went with GPL? We usually default to MIT unless good reason to do something else. Just curious :)

also, docs https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Licensing i think suggest that what you have (file LICENSE) isn't quite valid

Imports

  • You have 3 packges listed as imports, but you don't have roxygen import statements in your package.
    The most obvious way to see this is that your imports aren't listed in your NAMESPACE file. Use @importFrom pkg fxn1 fxn2 ... to import just the functions you need, then when you run document() or similar it will generate the namespace file for you, including imports. I usually put all these statements in the package level man file, which you have rgeospatialquality.R. See https://github.com/ropensci/rgbif/blob/master/R/rgbif-package.r#L28-L41 for example
  • It appears that you perhaps don't use rgbif in your package? If so, put it in Suggests, see
    related problem during check:
Namespace in Imports field not imported from:rgbifAll declared Imports should be used.

If rgbif is moved to Suggests, make sure to use it conditionally in your examples, vignettes, etc. That is, e.g.

if (requireNamespace("rgbif", quietly = TRUE)) {
  library("rgbif")
  ## your example code
} else {
  ## your example code if rgbif not available, or skip this else if rgbif required for example
}

Continuous integration

Nice work having Travis already set up. I'd also suggest using Appveyor-CI, see devtools::use_appveyor(). I forked your package and ran on Appveyor, tehre are

I got a failing test on windows, see https://ci.appveyor.com/project/sckott/rgeospatialquality/build/1.0.1#L793, but i'm not sure if that will be fixed when your Imports are structued correctly or not.

Docs

look good

Tests

  • All tests pass. nice. coverage is at about 65%, pretty good, but work on getting it up through time.
  • I imagine add_flags() is a good candidate for an S3 class with methods for at least
    data.frame and list. you have examples for data.frame's. Is there any use case where users
    would have lists instead of data.frame's? If so, could have add_flags.data.frame() and
    add_flags.list()
  • I'd like to see more tests especially covering how the package should fail, so e.g. using testthat::expect_error or similar to indicate how the functions should fail, e.g., what error messages the functions should give, whether they give back no content or they do and what it should be when they fail. Some example behavior I'm not quite sure I understand:

What does it mean to pass no inputs and get back data?

parse_record()
#> $hasCoordinates
#> [1] FALSE
#> 
#> $hasScientificName
#> [1] FALSE
#> 
#> $hasCountry
#> [1] FALSE

Examples

The examples are a bit sparse. You should add more, covering the minimum set of
different usage scenarios with the various parameter inputs.

Specific comments about the scripts and functions:

  • curl options: my personal preference is that all functions that make http requests allow users
    to pass in curl options (e.g,. verbose, progress, timeout, etc.). I'd recommend adding this
    ability to the appropriate fxns. I usually do this with ..., then users can just use httr
    curl options as if they were calling GET/POST directly, but if ... isn't possible, then
    use a named parameter.
  • you use NA for default parameters in your functions. I think it's better practice to
    use NULL instead. though maybe you had a good reason to use NA? You can easily then remove
    all NULL params input before passing into GET or POST query by using a utility function
    plyr::compact, all it is is function (x) Filter(Negate(is.null), x), which you can just
    put in your package
  • format_gq()
    • I would provide a quiet or verbose parameter so that the messages
      can be silenced in case the user wants to do that.
    • you say the indf parameter is required, however you have a default of NA for that
      parameter. Just use indf in the function definition, so users that don't pass that
      parameter get an error that it's missing
    • You should check user inputs to the source parameter, e.g., format_gq(d, source="asdf")
      doesn't fail well. Use e.g., match.arg() or something similar
    • I think in addition to supplying source parameter you could auto-detect
      the data source (gbif, vertnet, etc.) at least with those data sources that include
      class info or other info that uniquely identifies where it came from. And if auto-detection
      of source fails you could error with message
  • parse_record()
    • at the top of the function you have if (!missing(record)), but you have the default for
      that param set to NA, so how could it be missing? See also above comment about switching
      to NULL as defaults, and if you change to NULL, the helper function gq_parse_params()
      I think is just replaced by the compact() function above, which just removes any that are
      NULL
    • Users may pass in a list to the first parameter as well as inputs to the others. You
      could check internally whether both are passed, and stop with informative message if they are
  • add_flags()
    • same comment above for indf parameter, if required, don't give a default
  • gq_parse has code for parsing error messages. would be nice to see tests showing
    what is expected in different scenarios
  • I wonder if you could provide some flexibility to users in the required fields input
    to add_flags(), e.g., you could check for decimalLatitude ignoring case, and even accept something
    like latitude or lat. Not sure if there's limitations here due to the API though.

Code of conduct:

  • do add a code of conduct, see devtools::use_code_of_conduct()

est. hours

~5

Member

sckott commented Mar 24, 2016

Nice package! Overall, this is great! Some suggestions for improvements detailed below.

DESCRIPTION file

  • Title field: you have R Wrapper for the Geospatial Data Quality REST API. Just change to Wrapper for the Geospatial Data Quality REST API, as they sometimes complain about the use of R in the title and description, since they say it's obvious it's for R since its on CRAN.
  • Description field: you have This package provides native wrappers for the functions available via de spatial quality REST API. More information on the API can be found here: http://bit.ly/bioinformatics_btw057.. Change de to the

Installation

Works on all platforms, windows and linux installs pass.

travis file

I suggest removing

r_github_packages:
  - jimhester/covr

and put covr in Suggests in DESCRIPTION file

license

Any reason you went with GPL? We usually default to MIT unless good reason to do something else. Just curious :)

also, docs https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Licensing i think suggest that what you have (file LICENSE) isn't quite valid

Imports

  • You have 3 packges listed as imports, but you don't have roxygen import statements in your package.
    The most obvious way to see this is that your imports aren't listed in your NAMESPACE file. Use @importFrom pkg fxn1 fxn2 ... to import just the functions you need, then when you run document() or similar it will generate the namespace file for you, including imports. I usually put all these statements in the package level man file, which you have rgeospatialquality.R. See https://github.com/ropensci/rgbif/blob/master/R/rgbif-package.r#L28-L41 for example
  • It appears that you perhaps don't use rgbif in your package? If so, put it in Suggests, see
    related problem during check:
Namespace in Imports field not imported from:rgbifAll declared Imports should be used.

If rgbif is moved to Suggests, make sure to use it conditionally in your examples, vignettes, etc. That is, e.g.

if (requireNamespace("rgbif", quietly = TRUE)) {
  library("rgbif")
  ## your example code
} else {
  ## your example code if rgbif not available, or skip this else if rgbif required for example
}

Continuous integration

Nice work having Travis already set up. I'd also suggest using Appveyor-CI, see devtools::use_appveyor(). I forked your package and ran on Appveyor, tehre are

I got a failing test on windows, see https://ci.appveyor.com/project/sckott/rgeospatialquality/build/1.0.1#L793, but i'm not sure if that will be fixed when your Imports are structued correctly or not.

Docs

look good

Tests

  • All tests pass. nice. coverage is at about 65%, pretty good, but work on getting it up through time.
  • I imagine add_flags() is a good candidate for an S3 class with methods for at least
    data.frame and list. you have examples for data.frame's. Is there any use case where users
    would have lists instead of data.frame's? If so, could have add_flags.data.frame() and
    add_flags.list()
  • I'd like to see more tests especially covering how the package should fail, so e.g. using testthat::expect_error or similar to indicate how the functions should fail, e.g., what error messages the functions should give, whether they give back no content or they do and what it should be when they fail. Some example behavior I'm not quite sure I understand:

What does it mean to pass no inputs and get back data?

parse_record()
#> $hasCoordinates
#> [1] FALSE
#> 
#> $hasScientificName
#> [1] FALSE
#> 
#> $hasCountry
#> [1] FALSE

Examples

The examples are a bit sparse. You should add more, covering the minimum set of
different usage scenarios with the various parameter inputs.

Specific comments about the scripts and functions:

  • curl options: my personal preference is that all functions that make http requests allow users
    to pass in curl options (e.g,. verbose, progress, timeout, etc.). I'd recommend adding this
    ability to the appropriate fxns. I usually do this with ..., then users can just use httr
    curl options as if they were calling GET/POST directly, but if ... isn't possible, then
    use a named parameter.
  • you use NA for default parameters in your functions. I think it's better practice to
    use NULL instead. though maybe you had a good reason to use NA? You can easily then remove
    all NULL params input before passing into GET or POST query by using a utility function
    plyr::compact, all it is is function (x) Filter(Negate(is.null), x), which you can just
    put in your package
  • format_gq()
    • I would provide a quiet or verbose parameter so that the messages
      can be silenced in case the user wants to do that.
    • you say the indf parameter is required, however you have a default of NA for that
      parameter. Just use indf in the function definition, so users that don't pass that
      parameter get an error that it's missing
    • You should check user inputs to the source parameter, e.g., format_gq(d, source="asdf")
      doesn't fail well. Use e.g., match.arg() or something similar
    • I think in addition to supplying source parameter you could auto-detect
      the data source (gbif, vertnet, etc.) at least with those data sources that include
      class info or other info that uniquely identifies where it came from. And if auto-detection
      of source fails you could error with message
  • parse_record()
    • at the top of the function you have if (!missing(record)), but you have the default for
      that param set to NA, so how could it be missing? See also above comment about switching
      to NULL as defaults, and if you change to NULL, the helper function gq_parse_params()
      I think is just replaced by the compact() function above, which just removes any that are
      NULL
    • Users may pass in a list to the first parameter as well as inputs to the others. You
      could check internally whether both are passed, and stop with informative message if they are
  • add_flags()
    • same comment above for indf parameter, if required, don't give a default
  • gq_parse has code for parsing error messages. would be nice to see tests showing
    what is expected in different scenarios
  • I wonder if you could provide some flexibility to users in the required fields input
    to add_flags(), e.g., you could check for decimalLatitude ignoring case, and even accept something
    like latitude or lat. Not sure if there's limitations here due to the API though.

Code of conduct:

  • do add a code of conduct, see devtools::use_code_of_conduct()

est. hours

~5

@jotegui

This comment has been minimized.

Show comment
Hide comment
@jotegui

jotegui Mar 28, 2016

Thanks for the comments, @sckott. Really useful! I will start working on them and will update this thread when finished. Shouldn't take too long.

jotegui commented Mar 28, 2016

Thanks for the comments, @sckott. Really useful! I will start working on them and will update this thread when finished. Shouldn't take too long.

@sckott

This comment has been minimized.

Show comment
Hide comment
@sckott

sckott Mar 28, 2016

Member

great, thanks!

Member

sckott commented Mar 28, 2016

great, thanks!

@jotegui

This comment has been minimized.

Show comment
Hide comment
@jotegui

jotegui Apr 1, 2016

Hi @sckott. I have a new version ready (v0.3.0) addressing your suggestions. I'll go over them sequentially. Please let me know if I need to do anything else for the review.

Thanks!


  • Title field: you have R Wrapper for the Geospatial Data Quality REST API. Just change to Wrapper for the Geospatial Data Quality REST API, as they sometimes complain about the use of R in the title and description, since they say it's obvious it's for R since its on CRAN.

done

  • Description field: you have This package provides native wrappers for the functions available via de spatial quality REST API. More information on the API can be found here: http://bit.ly/bioinformatics_btw057.. Change de to the

done

I suggest removing

r_github_packages:
  - jimhester/covr

and put covr in Suggests in DESCRIPTION file

done

Any reason you went with GPL? We usually default to MIT unless good reason to do something else. Just curious :)
also, docs https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Licensing i think suggest that what you have (file LICENSE) isn't quite valid

Not really. It's just the license I have been using so far for my other projects. Happy to change it to MIT, for intra-ropensci consistency's sake :) I have adapted the DESCRIPTION file accordingly.

  • You have 3 packges listed as imports, but you don't have roxygen import statements in your package.

fixed. As you suggest, I have added @importFrom statements in the rgeospatialquality.R file

  • It appears that you perhaps don't use rgbif in your package? If so, put it in Suggests, see related problem during check:

done. I followed your suggestion and use requireNamespace each time rgbif (or any other data source package) is used. Now (as you will see below and in the package's README, both Travis and Appveyor pass.

I'd also suggest using Appveyor-CI

done

I got a failing test on windows, see https://ci.appveyor.com/project/sckott/rgeospatialquality/build/1.0.1#L793, but i'm not sure if that will be fixed when your Imports are structued correctly or not.

fixed. I have successfully integrated Appveyor now

  • All tests pass. nice. coverage is at about 65%, pretty good, but work on getting it up through time.

Current coverage is 93%

  • I imagine add_flags() is a good candidate for an S3 class with methods for at least data.frame and list. you have examples for data.frame's. Is there any use case where users would have lists instead of data.frame's? If so, could have add_flags.data.frame() and add_flags.list()

It is probably because of my (almost total) lack of understanding of how S3 classes work, but I can't see how can this improve usability of the function. In principle, add_flags is meant to work with datasets of more than 1 record, and therefore, the structure that would fit best here is the data.frame. Aren't lists designed just for the equivalent of a single record? If so, parse_record would be the best function to use here... But, as I said, it might be my lack of understanding, so if you can help me seeing it better, I can definitely work on turning this an S3 class :)

  • I'd like to see more tests especially covering how the package should fail (...)

Due to the flexible nature of the underlying API, it is actually difficult to make it fail, and there is almost none error handling currently implemented. However, I have added some warnings and improved docs to reflect this.

What does it mean to pass no inputs and get back data?

parse_record()
#> $hasCoordinates
#> [1] FALSE
#> 
#> $hasScientificName
#> [1] FALSE
#> 
#> $hasCountry
#> [1] FALSE

I have added warning messages and docs explaining this more generally. In this particular case, we wanted the API to be flexible and, even thought this is a feasible call, it is totally useless.

The examples are a bit sparse

done

  • curl options: my personal preference is that all functions that make http requests allow users to pass in curl options (e.g,. verbose, progress, timeout, etc.). I'd recommend adding this ability to the appropriate fxns. I usually do this with ..., then users can just use httr curl options as if they were calling GET/POST directly, but if ... isn't possible, then use a named parameter.

done, added ... to every httr calling function

  • you use NA for default parameters in your functions. I think it's better practice to use NULL instead. though maybe you had a good reason to use NA?

done. Not any particular reason to use NA

format_gq

- I would provide a `quiet` or `verbose` parameter so that the messages can be silenced in case the user wants to do that.

done

- you say the `indf` parameter is required, however you have a default of `NA` for that parameter. Just use `indf` in the function definition, so users that don't pass that parameter get an error that it's missing

done

- You should check user inputs to the `source` parameter, e.g., `format_gq(d, source=\"asdf\")` doesn't fail well. Use e.g., `match.arg()` or something similar

done

- I think in addition to supplying `source` parameter you could auto-detect the data source (gbif, vertnet, etc.) at least with those data sources that include class info or other info that uniquely identifies where it came from. And if auto-detection of source fails you could error with message

I also thought about it, but the problem is that (among the tested packages) only rgbif shows any distinctive info when calling class(obj). The other two (rvertnet and rinat) only return data.frame or list. And I don't really feel comfortable relying on the data.frames' headers to figure it out, since that is likely to change. If it is a really helpful addition, I can do that, but it will probably only work for rgbif

  • parse_record()
    • at the top of the function you have if (!missing(record)), but you have the default for that param set to NA, so how could it be missing?

fixed

See also above comment about switching to NULL as defaults

done

- Users may pass in a list to the first parameter as well as inputs to the others. You could check internally whether both are passed, and stop with informative message if they are

fixed

  • add_flags()
    • same comment above for indf parameter, if required, don't give a default

done

  • gq_parse has code for parsing error messages. would be nice to see tests showing what is expected in different scenarios

See comment above re: making the API fail. It is difficult to create scenarios where the API might throw an error message...

  • I wonder if you could provide some flexibility to users in the required fields input to add_flags(), e.g., you could check for decimalLatitude ignoring case, and even accept something like latitude or lat. Not sure if there's limitations here due to the API though.

done. Added a new parameter to add_flags(), called guess_fields, that does this

  • do add a code of conduct, see devtools::use_code_of_conduct()

done

jotegui commented Apr 1, 2016

Hi @sckott. I have a new version ready (v0.3.0) addressing your suggestions. I'll go over them sequentially. Please let me know if I need to do anything else for the review.

Thanks!


  • Title field: you have R Wrapper for the Geospatial Data Quality REST API. Just change to Wrapper for the Geospatial Data Quality REST API, as they sometimes complain about the use of R in the title and description, since they say it's obvious it's for R since its on CRAN.

done

  • Description field: you have This package provides native wrappers for the functions available via de spatial quality REST API. More information on the API can be found here: http://bit.ly/bioinformatics_btw057.. Change de to the

done

I suggest removing

r_github_packages:
  - jimhester/covr

and put covr in Suggests in DESCRIPTION file

done

Any reason you went with GPL? We usually default to MIT unless good reason to do something else. Just curious :)
also, docs https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Licensing i think suggest that what you have (file LICENSE) isn't quite valid

Not really. It's just the license I have been using so far for my other projects. Happy to change it to MIT, for intra-ropensci consistency's sake :) I have adapted the DESCRIPTION file accordingly.

  • You have 3 packges listed as imports, but you don't have roxygen import statements in your package.

fixed. As you suggest, I have added @importFrom statements in the rgeospatialquality.R file

  • It appears that you perhaps don't use rgbif in your package? If so, put it in Suggests, see related problem during check:

done. I followed your suggestion and use requireNamespace each time rgbif (or any other data source package) is used. Now (as you will see below and in the package's README, both Travis and Appveyor pass.

I'd also suggest using Appveyor-CI

done

I got a failing test on windows, see https://ci.appveyor.com/project/sckott/rgeospatialquality/build/1.0.1#L793, but i'm not sure if that will be fixed when your Imports are structued correctly or not.

fixed. I have successfully integrated Appveyor now

  • All tests pass. nice. coverage is at about 65%, pretty good, but work on getting it up through time.

Current coverage is 93%

  • I imagine add_flags() is a good candidate for an S3 class with methods for at least data.frame and list. you have examples for data.frame's. Is there any use case where users would have lists instead of data.frame's? If so, could have add_flags.data.frame() and add_flags.list()

It is probably because of my (almost total) lack of understanding of how S3 classes work, but I can't see how can this improve usability of the function. In principle, add_flags is meant to work with datasets of more than 1 record, and therefore, the structure that would fit best here is the data.frame. Aren't lists designed just for the equivalent of a single record? If so, parse_record would be the best function to use here... But, as I said, it might be my lack of understanding, so if you can help me seeing it better, I can definitely work on turning this an S3 class :)

  • I'd like to see more tests especially covering how the package should fail (...)

Due to the flexible nature of the underlying API, it is actually difficult to make it fail, and there is almost none error handling currently implemented. However, I have added some warnings and improved docs to reflect this.

What does it mean to pass no inputs and get back data?

parse_record()
#> $hasCoordinates
#> [1] FALSE
#> 
#> $hasScientificName
#> [1] FALSE
#> 
#> $hasCountry
#> [1] FALSE

I have added warning messages and docs explaining this more generally. In this particular case, we wanted the API to be flexible and, even thought this is a feasible call, it is totally useless.

The examples are a bit sparse

done

  • curl options: my personal preference is that all functions that make http requests allow users to pass in curl options (e.g,. verbose, progress, timeout, etc.). I'd recommend adding this ability to the appropriate fxns. I usually do this with ..., then users can just use httr curl options as if they were calling GET/POST directly, but if ... isn't possible, then use a named parameter.

done, added ... to every httr calling function

  • you use NA for default parameters in your functions. I think it's better practice to use NULL instead. though maybe you had a good reason to use NA?

done. Not any particular reason to use NA

format_gq

- I would provide a `quiet` or `verbose` parameter so that the messages can be silenced in case the user wants to do that.

done

- you say the `indf` parameter is required, however you have a default of `NA` for that parameter. Just use `indf` in the function definition, so users that don't pass that parameter get an error that it's missing

done

- You should check user inputs to the `source` parameter, e.g., `format_gq(d, source=\"asdf\")` doesn't fail well. Use e.g., `match.arg()` or something similar

done

- I think in addition to supplying `source` parameter you could auto-detect the data source (gbif, vertnet, etc.) at least with those data sources that include class info or other info that uniquely identifies where it came from. And if auto-detection of source fails you could error with message

I also thought about it, but the problem is that (among the tested packages) only rgbif shows any distinctive info when calling class(obj). The other two (rvertnet and rinat) only return data.frame or list. And I don't really feel comfortable relying on the data.frames' headers to figure it out, since that is likely to change. If it is a really helpful addition, I can do that, but it will probably only work for rgbif

  • parse_record()
    • at the top of the function you have if (!missing(record)), but you have the default for that param set to NA, so how could it be missing?

fixed

See also above comment about switching to NULL as defaults

done

- Users may pass in a list to the first parameter as well as inputs to the others. You could check internally whether both are passed, and stop with informative message if they are

fixed

  • add_flags()
    • same comment above for indf parameter, if required, don't give a default

done

  • gq_parse has code for parsing error messages. would be nice to see tests showing what is expected in different scenarios

See comment above re: making the API fail. It is difficult to create scenarios where the API might throw an error message...

  • I wonder if you could provide some flexibility to users in the required fields input to add_flags(), e.g., you could check for decimalLatitude ignoring case, and even accept something like latitude or lat. Not sure if there's limitations here due to the API though.

done. Added a new parameter to add_flags(), called guess_fields, that does this

  • do add a code of conduct, see devtools::use_code_of_conduct()

done

@sckott

This comment has been minimized.

Show comment
Hide comment
@sckott

sckott Apr 2, 2016

Member

thanks, i'll have a look soon and get back to you here

Member

sckott commented Apr 2, 2016

thanks, i'll have a look soon and get back to you here

@jotegui

This comment has been minimized.

Show comment
Hide comment
@jotegui

jotegui Apr 2, 2016

Awesome, many thanks!

Javier Otegui
http://www.jotegui.com
El 02/04/2016 21:13, "Scott Chamberlain" notifications@github.com
escribió:

thanks, i'll have a look soon and get back to you here


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#28 (comment)

jotegui commented Apr 2, 2016

Awesome, many thanks!

Javier Otegui
http://www.jotegui.com
El 02/04/2016 21:13, "Scott Chamberlain" notifications@github.com
escribió:

thanks, i'll have a look soon and get back to you here


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#28 (comment)

@sckott

This comment has been minimized.

Show comment
Hide comment
@sckott

sckott Apr 7, 2016

Member

Thanks for all the changes. Just commenting on those that require responses:

Happy to change it to MIT, for intra-ropensci consistency's sake :) I have adapted the DESCRIPTION file accordingly.

thanks!

It is probably because of my (almost total) lack of understanding of how S3 classes work, but I can't see how can this improve usability of the function. In principle, add_flags is meant to work with datasets of more than 1 record, and therefore, the structure that would fit best here is the data.frame. Aren't lists designed just for the equivalent of a single record? If so, parse_record would be the best function to use here... But, as I said, it might be my lack of understanding, so if you can help me seeing it better, I can definitely work on turning this an S3 class :)

lists can be of any length, and nested as well. I don't think you have to support lists, but if you only want to support data.frame's then I'd make an S3 class so that the function fails nicely for users that try to pass other classes into it. e.g.,

foo <- function(x) {
    UseMethod("foo")
}

foo.data.frame <- function(x) {
    # do stuff
}

foo.default <- function(x) {
    stop("foo does not support input of class ", class(x), call. = FALSE)
}

I also thought about it, but the problem is that (among the tested packages) only rgbif shows any distinctive info when calling class(obj). ...

Okay, makes sense

Member

sckott commented Apr 7, 2016

Thanks for all the changes. Just commenting on those that require responses:

Happy to change it to MIT, for intra-ropensci consistency's sake :) I have adapted the DESCRIPTION file accordingly.

thanks!

It is probably because of my (almost total) lack of understanding of how S3 classes work, but I can't see how can this improve usability of the function. In principle, add_flags is meant to work with datasets of more than 1 record, and therefore, the structure that would fit best here is the data.frame. Aren't lists designed just for the equivalent of a single record? If so, parse_record would be the best function to use here... But, as I said, it might be my lack of understanding, so if you can help me seeing it better, I can definitely work on turning this an S3 class :)

lists can be of any length, and nested as well. I don't think you have to support lists, but if you only want to support data.frame's then I'd make an S3 class so that the function fails nicely for users that try to pass other classes into it. e.g.,

foo <- function(x) {
    UseMethod("foo")
}

foo.data.frame <- function(x) {
    # do stuff
}

foo.default <- function(x) {
    stop("foo does not support input of class ", class(x), call. = FALSE)
}

I also thought about it, but the problem is that (among the tested packages) only rgbif shows any distinctive info when calling class(obj). ...

Okay, makes sense

@sckott

This comment has been minimized.

Show comment
Hide comment
@sckott

sckott Apr 7, 2016

Member

@jotegui after resolution of the above comment about S3 - we're ready to move it to ropensci

Member

sckott commented Apr 7, 2016

@jotegui after resolution of the above comment about S3 - we're ready to move it to ropensci

@jotegui

This comment has been minimized.

Show comment
Hide comment
@jotegui

jotegui Apr 8, 2016

Hi @sckott

Thanks for your answer, and for the hint on S3 classes and methods. I have now updated the code and implemented both add_flags and format_gq as S3 classes, with methods only for data.frames. I thought it was better to release this early and, later on, I can always add new methods for different object types, like lists, right?

Also, I have added a bunch of tests for this new setup and fixed some weird dependency issues with travis. I had to add the following lines to .travis.yml to make sure it finds the packages under Suggests:

repos:
  CRAN: https://cloud.r-project.org
  ropensci: http://packages.ropensci.org
r_github_packages:
  - ropensci/rgbif
  - ropensci/rvertnet
  - ropensci/rinat

Let me know if I need to do something else.

Cheers!

jotegui commented Apr 8, 2016

Hi @sckott

Thanks for your answer, and for the hint on S3 classes and methods. I have now updated the code and implemented both add_flags and format_gq as S3 classes, with methods only for data.frames. I thought it was better to release this early and, later on, I can always add new methods for different object types, like lists, right?

Also, I have added a bunch of tests for this new setup and fixed some weird dependency issues with travis. I had to add the following lines to .travis.yml to make sure it finds the packages under Suggests:

repos:
  CRAN: https://cloud.r-project.org
  ropensci: http://packages.ropensci.org
r_github_packages:
  - ropensci/rgbif
  - ropensci/rvertnet
  - ropensci/rinat

Let me know if I need to do something else.

Cheers!

@sckott

This comment has been minimized.

Show comment
Hide comment
@sckott

sckott Apr 8, 2016

Member

I have now updated the code and implemented both add_flags and format_gq as S3 classes, with methods only for data.frames

great, thanks

I thought it was better to release this early and, later on, I can always add new methods for different object types, like lists, right?

Yeah, easy to add functionality later

Keep in mind with Remotes this https://github.com/hadley/devtools/blob/master/vignettes/dependencies.Rmd#cran-submission

Member

sckott commented Apr 8, 2016

I have now updated the code and implemented both add_flags and format_gq as S3 classes, with methods only for data.frames

great, thanks

I thought it was better to release this early and, later on, I can always add new methods for different object types, like lists, right?

Yeah, easy to add functionality later

Keep in mind with Remotes this https://github.com/hadley/devtools/blob/master/vignettes/dependencies.Rmd#cran-submission

@sckott

This comment has been minimized.

Show comment
Hide comment
@sckott

sckott Apr 8, 2016

Member

I think we're all set - I invited you to our organization ropenscilabs - we start newer pkgs there now, then move to ropensci when pkgs are more stable - After you accept the invitation, you should be able to transfer the repo, let me know if that doesn't work

Member

sckott commented Apr 8, 2016

I think we're all set - I invited you to our organization ropenscilabs - we start newer pkgs there now, then move to ropensci when pkgs are more stable - After you accept the invitation, you should be able to transfer the repo, let me know if that doesn't work

@jotegui

This comment has been minimized.

Show comment
Hide comment
@jotegui

jotegui Apr 8, 2016

I'm on it. I should give team access just to rgeospatialqual, not to unconf-team, right?

jotegui commented Apr 8, 2016

I'm on it. I should give team access just to rgeospatialqual, not to unconf-team, right?

@sckott

This comment has been minimized.

Show comment
Hide comment
@sckott

sckott Apr 8, 2016

Member

Right

Member

sckott commented Apr 8, 2016

Right

@sckott

This comment has been minimized.

Show comment
Hide comment
@sckott

sckott Apr 8, 2016

Member

@jotegui Thanks for your submission!

Member

sckott commented Apr 8, 2016

@jotegui Thanks for your submission!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment