ukgovdatascience / govstyle

Theme for use with ggplot2 for creating government style visualisations
http://ukgovdatascience.github.io/govstyle/index.html
GNU General Public License v3.0
63 stars 12 forks source link

Add CONTRIBUTING and explanation about packrat #20

Closed ivyleavedtoadflax closed 5 years ago

ivyleavedtoadflax commented 7 years ago

Following #19, add some explanation about the use of packrat for contributors in CONTRIBUTING.md and README.md.

codecov[bot] commented 7 years ago

Codecov Report

Merging #20 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #20   +/-   ##
=======================================
  Coverage   80.43%   80.43%           
=======================================
  Files           2        2           
  Lines          46       46           
=======================================
  Hits           37       37           
  Misses          9        9

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a58c163...2f7a5ed. Read the comment docs.

nacnudus commented 5 years ago

Closing because I don't think packrat helps users in the way intended. https://github.com/ukgovdatascience/govstyle/pull/26 removes packrat.

Packrat manages dependencies, not compatibility. Using packrat for the sake of backwards compatibility only creates a new problem: forwards compatibility. install_github() doesn't refer to packrat so doesn't reproduce the environment that this package has been developed in, so users only benefit if their existing environment happens to be compatible with the one configured here.

One way to manage compatibility is to put this package on CRAN, so that maintainers its dependencies would be obliged to check their own packages' backwards compatibility with this one when submitting updates.

ivyleavedtoadflax commented 5 years ago

I think you have misunderstood the reason for using packrat here. Many govt users are not in the 'normal' situation where they can upgrade and downgrade versions of packages at will. In some departments, users are reliant on IT auditing versions of packages, and actually mirroring a snapshot of CRAN on internal hardware. If I had developed govstyle on a development environment with all the latest updates to all packages, it was conceivable that the package itself would not be compatible with older versions of its dependencies that were available to users on restricted IT systems.

That was the motivation behind packrat - to deliberately retard the development environment, NOT to manage the package dependencies, which would not have happened anyway with install_github() as you say. I think this is a legitimate use of packrat.

With limitations on govt IT being relaxed it is probably legitimate to stop using packrat, but you should be careful that you don't break builds for legacy users. This is certainly a problem we ran into in the development of eesectors where the use of packrat to replicate the restricted IT environment was essentially to getting a working build. I don't remember if it was caused by govstyle.

nacnudus commented 5 years ago

But what about users who do have up-to-date packages that have (potentially) deprecated the functions used here?

ivyleavedtoadflax commented 5 years ago

I think the correct answer is that legacy users install older versions of the package. And I agree, catering for current (up-to-date) users should be the main concern. I just wanted to give you context on why I made these decisions in the past. Also, this isn't my problem anymore :trollface: :parasol_on_ground: :cocktail:

nacnudus commented 5 years ago

Fair enough -- how about if I do a 'release' of the current version and put installation instructions in the README, similar to https://github.com/nacnudus/unpivotr#installation?

You've got me thinking about how to set up Travis to test inside a packrat environment, so thanks for that nerd snipe!

ivyleavedtoadflax commented 5 years ago

Haha any time. Sounds like a plan. I'm pretty sure we had this set up to run on a packrat cache for one of these repos. But not sure which one now, maybe eesectors.

ivyleavedtoadflax commented 5 years ago

Yes, see https://github.com/DCMSstats/eesectors/blob/master/.travis.yml

nacnudus commented 5 years ago

Thanks! There's also some new configuration available https://docs.travis-ci.com/user/languages/r/#packrat It doesn't seem possible to configure two builds: one with the latest packages and the other with the packrat cache.

install:
  - R -e "0" --args --bootstrap-packrat
  - R -e "packrat::restore(restart = FALSE)"