ropensci / neotoma

Programmatic R interface to the Neotoma Paleoecological Database.
https://docs.ropensci.org/neotoma
Other
30 stars 16 forks source link

Myriad fixes to get neotoma to build & pass checks #16

Closed gavinsimpson closed 11 years ago

gavinsimpson commented 11 years ago

Neotoma needed several fixes to get it to build and pass R CMD check on my Linux box.

  1. There was a non-ASCII character in R/get_contacts.R.
  2. A couple of files had conflict markers from the VCS. I was able to infer the intention as the conflicts were minor. Files affected R/get_download.R, R/get_taxa.R, man/get_contacts.Rd, and man/get_contacts.Rd
  3. Finally, I'm not sure the intention here but several of the R files were referencing a variable neotoma.form in

    if(class(aa) == 'try-error'){ output <- neotoma.form }

    but this object was not defined. Looking back through the sources I noticed that some files had this

    neotoma.form <- getForm(base.uri, .params = cl)
    aa <- try(fromJSON(neotoma.form, nullValue=NA))

    whereas in other files you'd inlined this to

    aa <- try(fromJSON(getForm(base.uri, .params = cl), nullValue=NA))

    However, with the latter, there now is no neotoma.form to reference in the check for class "try-error". To fix this, I presumed that the intention of the code was to pass back the output of the getForm() call if the fromJSON() call failed. This may not be correct; you might have just wanted to pass aa on to output without further processing.

  4. In R/get_taxa.R you marked up a link in the Roxygen header using \link{}. That is for cross-references between packages, not for URI resources. For these you have two choices \href{uri}{in-text} or \url{uri} where uri is the link and in-text is the link text. I assumed you wanted the URI displayed so changed the markup to \url{}.

This is my first time doing any collaborative coding via GitHub so if I've misunderstood or made the wrong change let me know and I'll fix issues in my branch and send another pull request.

Cheers,

Gavin

SimonGoring commented 11 years ago

Thanks Gavin! Much appreciated. I think we're out of sync on these changes, but I don't have much experience so I have to pass this merge off to @SChamberlain or @karthikram. I'm going to make some more changes over the next few days, maybe you and I can chat?

sckott commented 11 years ago

Yep, I can merge. I'll have a look at the changes in the pull request

gavinsimpson commented 11 years ago

Yes, sorry about that - I forgot to update my fork and only just noticed that Scott had made quite a few changes between last evening and now. Looks like Scot got most of them - I didn't do a thorough check yet as I wanted to get back here and apologise for the pull request on an out of date version.

If you want to chat, fine with me. Use your social-media of choice :-) Should hopefully get to use neotoma in anger today!

sckott commented 11 years ago

@gavinsimpson No worries, should be no problem.

gavinsimpson commented 11 years ago

Looking a bit closer, I think you got changes 2 and 4.

Next time I'll do smaller commits - still using git like I do with SVN on my own repos!