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

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

Merged
merged 48 commits into from Jul 22, 2019
Merged

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

merged 48 commits into from Jul 22, 2019

Conversation

mcanouil
Copy link
Contributor

@mcanouil mcanouil commented Jul 22, 2019

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:

  • a CODECOV_TOKEN in travis ("Environment Variables")
  • a ssh key id_rsa to allow travis to write pkgdown generated files. (Repository "Deploy keys")

Changelog (ggnetwork 0.5.6)

Repository changes

  • Add README.Rmd (replace README.md),

    • use badges.
    • list all GitHub contributors.
    • add Contributing, Code of Conduct Issue Template and Support markdown file (usethis).
  • Improve .travis.yml,

    • use pkgdown for website deployment.
      • add _pkgdown.yml configuration file.
    • use covr for code coverage.
      • add default codecov.yml configuration file.
  • Add ggnetwork.Rproj, for ease of use within Rstudio.

Minor improvements and fixes

  • In DESCRIPTION,

    • update RoxygenNote version.
    • remove ggplot2 from Enhances field.
    • add Collate field to load sequentially the functions.
    • move igraph to Imports field.
  • Remove inst/doc/ directory, i.e., the vignette is part of the pkgdown website.

  • Use tidy code style.

  • In R/fortify-igraph.R and R/fortify-network.R,

    • use subsetting functions instead of with and transform (i.e., intended to be use interactively).
    • fix issue from CRAN check with undefined global variables.
    • remove namespace loading for sna package (i.e., gplot.layout.* functions).
  • In R/geom-nodes.R and R/geom-edges.R,

    • remove unnecessary "@importFom".
    • add missing function packages prefix.
  • In R/ggnetwork.R, switch and tryCatch to make the igraph and network testing consistent.

  • In R/utilities.R, reexport fortify and unit from ggplot2.

@mcanouil mcanouil changed the title Improve package structure (using tidy style) Improve package structure & coding (using tidy style) Jul 22, 2019
@briatte briatte merged commit b88cd0a into briatte:master Jul 22, 2019
@briatte
Copy link
Owner

briatte commented Jul 22, 2019

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
Copy link
Owner

briatte commented Jul 22, 2019

@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
Copy link
Owner

briatte commented Jul 23, 2019

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
Copy link
Owner

briatte commented Jul 23, 2019

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
Copy link
Contributor Author

mcanouil commented Jul 23, 2019

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).

@mcanouil mcanouil deleted the tidypkg branch July 23, 2019 07:15
@briatte
Copy link
Owner

briatte commented Jul 23, 2019

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
Copy link
Contributor Author

mcanouil commented Jul 23, 2019

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
Copy link
Contributor Author

mcanouil commented Jul 23, 2019

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
Copy link
Contributor Author

@briatte

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

ERROR: dependencystatnet.commonis not available for packagesna* removing/home/travis/R/Library/snaThe downloaded source packages are in/tmp/RtmpHHePGd/downloaded_packagesWarning messages:
1: packagestatnet.commonis 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

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.

None yet

2 participants