briatte / ggnetwork

Geoms to plot networks with ggplot2
https://briatte.github.io/ggnetwork/
146 stars 28 forks source link

Improve package structure & coding (using tidy style) #42

Closed mcanouil closed 5 years ago

mcanouil commented 5 years ago

I had a little bit of time to spend, but I was unable to see which issues were still open (i.e., some are from early ggnetwork version).

I "fixed" some part of the code which were "interactive" coding, some inconsistency with argument/variable naming leading to unexpected behaviour in some cases. Fix CRAN checks (no notes, warning, neither error).

Some changes are from #39

intergraph is referenced in ggnetwork.R#L11 without any import or dependency, is there any reasons for that?

The current PR add pkgdown website and code coverage.
To work, these changes need:

Changelog (ggnetwork 0.5.6)

Repository changes

Minor improvements and fixes

briatte commented 5 years ago

Hi @mcanouil

Merged, will inspect very soon! Thanks a lot, I'm very curious to take a look at how all those improvements work (I've never used pkgdown myself).

briatte commented 5 years ago

@mcanouil

Do you think this package is one where it would make sense to have

Enhances:
    ggplot2

in the DESCRIPTION file?

Wickham recommends against using Enhances, but it makes sense here, doesn't it?

Again, many thanks for your contributions on the package!

briatte commented 5 years ago

AppVeyor seems to work after a tiny fix to its YAML file.

The only warning I get from the log:

No artifacts found matching '*.Rcheck\**\*.fail' path

Also, what's the benefit of having both AppVeyor and Travis CI? Apologies if this should be obvious.

briatte commented 5 years ago

I've added a CODECOV_TOKEN to Travis CI.

However, while I did deploy a public SSH key to the repo, I'm failing to add the private SSH key to Travis CI. The docs do not seem to be up to date: I do not get a SSH upload form in my Travis CI settings, and the command line upload says "SSH keys are not available on travis-ci.org".

mcanouil commented 5 years ago

About the YAML you fixed. The build version template is defined through the UI. In that case it should look more like version: 0.1.0-{build} although only le "build" tag is increased each time.

Appveyor uses Windows virtual machine when Travis uses Linux-like machine. This is useful to check for CRAN upload to tests on both.

No artifact means the build did not find any of the pattern you were looking for. Since the patterns are warnings, notes or errors, this is a good sign.

“Enhances” field: my opinion is that a package should only be listed in one field. Since “depends” means the package cannot work without it and by definition a package is an extension, that means it supposed to enhance at least R.

Deploying the SSH key from the repo to travis is a bit tricky, i.e., I had to copy/paste several times the public the key to travis before it worked. Within the usethis package, there is a function to do that work (it did not work for me due to my server’s settings).

briatte commented 5 years ago

Re: AppVeyor -- alright, thanks for the details. The part I fixed in the YAML seems optional, so perhaps it should be left as is?

Re: Travis CI, I think I've managed to get the SSH keys where needed (using this for guidance and this for execution).

mcanouil commented 5 years ago

Re: Travis CI, I think I've managed to get the SSH keys where needed (using this for guidance and this for execution).

Yes I was talking about the use_* function.

By the way, you may noticed in the travis config file, I added tests for previous "minor" version of R to check if R (>= 3.1) was and remains true. The pkgdown website is deployed when R:release is succesful.

There is an issue due to "intergraph" package needed in your documention but not listed in at least Imports, which caused travis build to stop. Since you don't use any functions from that package, I would suggest to remove the link in ggnetwork /R/ggnetwork.R#L12

* checking Rd cross-references ... NOTE
Package unavailable to check Rd xrefs: ‘intergraph’
mcanouil commented 5 years ago

Pkgdown worked (if ".9000" the website is "under developement" and can be access withtin/dev) ! => https://briatte.github.io/ggnetwork/dev/

Once you want to release it, just remove ".9000", setup a tag in git, write a release post and send to CRAN (after travis and appveyor have succesfully run tests) and hopefully it will be accepted directly (most likely for update). The website for the current release will be available directly through https://briatte.github.io/ggnetwork/

mcanouil commented 5 years ago

@briatte

The current travis build does not work for all R version before 3.5 as you can see on travis

ERROR: dependency ‘statnet.common’ is not available for package ‘sna’
* removing ‘/home/travis/R/Library/sna’
The downloaded source packages are in
    ‘/tmp/RtmpHHePGd/downloaded_packages’
Warning messages:
1: package ‘statnet.common’ is not available (for R version 3.4.4) 

My opinion would be to use the following in DESCRIPTION:

Depends:
    R (>= 3.5),
    ggplot2 (>= 2.0.0)

Best regards