Improve DESCRIPTION

[dmpe]

Hi @tomschenkjr,
This is a second PR I have done. Basically, it cleans up the :: stuff. For reasons see here ropensci/software-review#17 (comment) (section Imports)

Additionally I have bumped up the version of the packages which are being used. If the user had httr version of 0.3, there would be no guarantee that it would work, because Hadley has changed a lot since then.

I hope you are ok with the fact that I have created issues for both of them. If needed feel free to do so.

PS: The two PR do not include themselves #46, so both need to be merged separately.

[tomschenkjr]

@dmpe -

Thanks!

@geneorama - can you look at the pkg1:: changes made in this PR. I know you implemented the :: syntax for preference, so interested in your thoughts.

[dmpe]

#42 to get rid of:

  Warning: replacing previous import by ‘httr::build_url’ when loading ‘RSocrata’
  Warning: replacing previous import by ‘httr::parse_url’ when loading ‘RSocrata’

[geneorama]

We should correct this too. We went from RJSONIO to rjson

[geneorama]

COMMENT ORIGINALLY POSTED IN #42 (sorry for the confusion)

Tom, thanks for bringing this to my attention.

Using importFrom is intentional, and from R documentation:
http://cran.r-project.org/doc/manuals/R-exts.html#Specifying-imports-and-exports

Using importFrom selectively rather than import is good practice.

Originally I didn't use importFrom just because it was good practice, I believe it solved another problem.

Also the use of :: is deliberate. Although I sometimes think it's useful in general scripts, I think it's very important to use in package development.

• It helps track where dependencies occur, which is useful in limiting the number of dependencies.
• It helps track where dependencies occur, which is useful in when possibly switching dependencies (e.g. switching between curl and RCurl)
• It avoids potential namespace conflicts that could be introduced by the order in which packages are attached
• It avoids namespace conflicts for potentially common function names (e.g. GET, which is currently an httr function, but could easily be defined elsewhere)
• It actually simplifies development (at the cost of complicating readability) because you no longer have to worry about having complicated namespace issues

Conceptually I like having dependencies that rely on the oldest packages possible, to avoid forcing someone to update for no reason. However, it's probably a good idea to update our versions, since this isn't something that I've been paying attention to (and I'm guessing nobody else has either). I wonder, does CRAN use the oldest package version possible when doing the checks?

[dmpe]

Conceptually I like having dependencies that rely on the oldest packages possible, to avoid forcing someone to update for no reason.

Yeeh. In R I would say that packages most of the times either fix, add or deprecate functionalities, instead of deleting them. So users can be actually very sure that functions can/will be deprecated but almost never deleted (unless functions are totally useless or bad/wrong).
This is what is very nice about R community.

I wonder, does CRAN use the oldest package version possible when doing the checks?

When running locally, maybe (--as-cran). When it is on CRAN, it is the newest ones, e.g. psych which is in development since 2005 and so MUCH has changed since then.

Using importFrom is intentional, and from R documentation.......

Correct. But both in the dev & master branch the statement

#' @importFrom pckg function

is not in the code. It is only in the namespace (which is kind of useless when you don't do statement above in the code directly).

Thus:

It avoids potential namespace conflicts that could be introduced by the order in which packages are attached
It avoids namespace conflicts for potentially common function names (e.g. GET, which is currently an httr function, but could easily be defined elsewhere)

are solved when writing #' @importFrom pckg function

To summarize, (and to paraphrase Scott) httr and jsonlite are listed under Imports in DESCRIPTION file, but they aren't listed in any @import statement. Even though httr and jsonlite are namespaced, those will still fail for users that don't have those pkgs installed.
source

(i had previously commited only @import, this has been now fixed to @importFrom with no longer getting an error and without curl)

It helps track where dependencies occur

Agree

...which is useful in limiting the number of dependencies.

Out of the interest, why ? #43 says it would be nice to use geojson. Well in that case https://github.com/ropensci/geojsonio will be required, with much bigger dependencies on rgdal and rgeos.

"complicating readability"

Not sure if I understand it correctly, but jsonlite:fromJson is actually easier to read than fromJson, because one automatically understands from which package it comes from.

[geneorama]

@dmpe I'm going to reply by topic, I think quoting again would be too complicated

Package version update: I think we're agreed, thanks for the suggestion to update. And it's surprising that CRAN would test using the most recent package version, that doesn't seem like them at all.

Namespace / importFrom / import:
It took me a while to understand what you were talking about, but now I think I see the disconnect.

I've manually created the NAMESPACE and DESCRIPTION files by manually searching for dependencies in the code. Currently, I have no problems, testing / building / installing (although I think we've been missing a step in generating documentation).

Whereas I think you're creating NAMESPACE (and maybe DESCRIPTION) using roxygen2's built in automagical functionality. (see your .Rproj options) This method probably fails without some sort of import statement in the scripts.

Although it seems like the functionality is a good idea here, I would like to have a better understanding of what it means to enable PackageUseDevtools: in the R Studio options.

Limiting dependencies I just meant that I don't want to have them if they're not needed. I especially want to avoid overlapping packages; e.g. it would be sloppy to continue to import RCurl if we're mostly using curl.

Complicating readability
Personally I think that jsonlite:fromJson is visually distracting compared to fromJson, but in RSocrata I would really prefer to always reference external libraries by their full name. Part of the reason is convenience. I like to step through the code without loading any libraries when doing some tests. To me, this is a nice way to make sure that I haven't missed any ::s, and the :: statements let me check if the import statements are correct. Plus, the ::s make it so that the code works, even if I don't have a library call at the beginning of the script.

Names (new item)
I noticed that the DESCRIPTION file has changed for Tom link. His role has changed from Author and Maintainer to "cre" and Maintainer, and it also dropped the Jr., which is part of his name. Is that being generated by devtools?

Thanks,

Gene

[geneorama]

BTW, I want to add that I think that including importFrom in the relevant functions could be a major improvement. It make a lot more sense to identify dependencies when they happen rather than collect them at the end. I just want to understand how it happens.

[dmpe]

to invs.

[dmpe]

Namespace / importFrom / import:
Ahh, ok. Now understand. Yes, namespace is always created using roxygen2 (at least it should be this way), but never the description file (this is a manual process). Due to the fact of having roxygen2 only in the suggest section (where it also belongs) the user is never asked to have this package installed too. It is basically only for developing purposes.

The ‘Suggests’ field uses the same syntax as ‘Depends’ and lists packages that are not necessarily needed. This includes packages used only in examples, tests or vignettes (see Writing package vignettes), and packages loaded in the body of functions.
http://cran.r-project.org/doc/manuals/r-release/R-exts.html (1.1.3)

PackageUseDevtools: in the R Studio options.
I think it only means that if you have devtools installed, then its functions (e.g. for testthat etc.) will be used.
==> devtools::test()

Names

Both ‘Author’ and ‘Maintainer’ fields can be omitted if a suitable ‘Authors@R’ field is given. This field can be used to provide a refined and machine-readable description of the package “authors” (in particular specifying their precise roles), via suitable R code. It should create an object of class "person', by either a call to person or a series of calls (one per “author”) concatenated by c()): see the example DESCRIPTION file above. The roles can include ‘"aut"’ (author) for full authors, ‘"cre"’ (creator) for the package maintainer, and ‘"ctb"’ (contributor) for other contributors, ‘"cph"’ (copyright holder), among others. See ?person for more information. Note that no role is assumed by default. Auto-generated package citation information takes advantage of this specification. The ‘Author’ and ‘Maintainer’ fields are auto-generated from it if needed when building5 or installing.

(source is the same as above)

(Sorry for confusion with "cre", "ctb" etc. This has been now improved.)

I also wonder, if you should be included too?

Import Namespaces ::

Plus, the ::s make it so that the code works, even if I don't have a library call at the beginning of the script.

But the point is that there should not be a library call library(httr) in the package. Unless for example it is for the examples. But even there one would use :

#' library(data)
#' funct(...)

But ok. On this point I think we can agree to disagree 😋

---

@tomschenkjr See here:
r-lib/devtools#527

Thus I will delete maintainer field, too

---

Build with an error because man dir is missing: https://travis-ci.org/dmpe/RSocrata/builds/70350743#L1085 -> fixed https://travis-ci.org/dmpe/RSocrata/builds/70351557

---

Good idea when merging would be to make all those commit into one commit.

[tomschenkjr]

We'll actually change this back to the old Author before pushing to CRAN. The CRAN guys are really picky that my email address in Authors@R match that of the Maintainer's email address. However, I like to separate the emails for general contact information from contacting me for package maintenance (CRAN will send e-mail blasts and I also use the email address in other projects I maintain). I prefer the Authors@R, but was getting cross with the CRAN submission process so used the old style.

[tomschenkjr]

Thank you for the work on this. Very useful contributions. We're going to merge this into the dev branch and finalize before uploading to CRAN. We're going to make one change in the DESCRIPTION file and I'll double check the remaining issue in #48.

[geneorama]

This is some clever stuff, even if it's no longer necessary

[geneorama]

@dmpe Thank you very much for your thoughtful, and very solid contributions. This is a big technical improvement.

The branch is merged into dev, the tests look good, I'll let @tomschenkjr merge into master (I think he still has other minor changes with the email address, etc.).

[dmpe]

@geneorama @tomschenkjr Great to hear that you liked that. 😃