pbs-assess / pacea

An R package to house Pacific Region ecosystem data to help facilitate an ecosystem approach to fisheries.
Other
14 stars 0 forks source link

For README.Rmd, maybe hard save the figures that won't be updated (when either of us commit they get updated but haven't changed). #44

Closed andrew-edwards closed 3 months ago

andrew-edwards commented 8 months ago

e.g. this commit 734b63 only added a badge (and updated the buoy number), but had lots of extraneous figures committed. Mention the workflow in Developers' Guidelines in README.

andrew-edwards commented 8 months ago

And if you just kill the changes to figures (i.e. ignore that they've changed), you get this error:

README.md is out of date; please re-knit README.Rmd
use 'git commit --no-verify' to override this check

The latter does override it, just a bit annoying to have to use.

seananderson commented 8 months ago

You should only get that if the .Rmd is newer than the .md. Look in .git/hooks/pre-commit if you want to disable that. The usethis package would have created it.

andrew-edwards commented 8 months ago

Thanks. Interesting. So much for telling people to "ignore the .git folder as you never need to touch it!".

So, for the record, I removed

if [[ README.Rmd -nt README.md ]]; then
  echo -e "README.md is out of date; please re-knit README.Rmd\n$MSG"
  exit 1
elif [[ ${#README[@]} -lt 2 ]]; then
  echo -e "README.Rmd and README.md should be both staged\n$MSG"
  exit 1
fi

and could commit an edited README.Rmd that was newer than README.Md.

  1. So presumably Travis would have to do the same thing locally, since .git/hooks don't get pushed: https://stackoverflow.com/questions/12222186/are-git-hooks-pushed-to-the-remote-when-i-git-push . But then how would Travis have that above code in his .git/hooks/pre-commit in the first place (assuming I first did the usethis? Strange...

  2. Still have to check (with Travis also) about the figures getting updated.

andrew-edwards commented 8 months ago

And to solve the original problem, we could maybe properly cache the figures and push them (need to look into exactly what happens now - we have fig.path and it doesn't remake figs every time, and then think about if this will fix the problem). Did a quick try of cache=TRUE but need to try tomorrow.

travistai2 commented 8 months ago

I just have the pre-commit.sample file. And I don't have that same text in my file as the one you deleted

andrew-edwards commented 8 months ago

Thanks.

Confusingly when I delete a .png file in man/figures, re-render the .Rmd, it creates the .png file again (with updated data), but still keeps the old timestamp of a month ago. I thought it might be doing something clever and the timestamp is to do with when the code chunk last changed (though I tried a test and that wasn't the case). I'll try removing the fig.path and add cache = TRUE to certain chunks.

andrew-edwards commented 8 months ago

Solution - the reason we committed the figures is to not require the README.Rmd (which build on github) to download the bccm data, which takes time. So I will just hardwire those example figures in, and not commit the plot(oni) ones that build quickly in the package. Think that should work... UPDATE - that's wrong, we build locally and push the .md (which gets converted to .html by github). Could set up GHA but that's a bit complex and risky I think.

andrew-edwards commented 8 months ago

Figured out a solution - working on it now.....

andrew-edwards commented 8 months ago

Works for me and I think it should for Travis, and I added the pre-commit text back in. Travis you should probably do the same, i.e. have this code

#!/bin/bash
README=($(git diff --cached --name-only | grep -Ei '^README\.[R]?md$'))
MSG="use 'git commit --no-verify' to override this check"

if [[ ${#README[@]} == 0 ]]; then
  exit 0
fi

if [[ README.Rmd -nt README.md ]]; then
  echo -e "README.md is out of date; please re-knit README.Rmd\n$MSG"
  exit 1
elif [[ ${#README[@]} -lt 2 ]]; then
  echo -e "README.Rmd and README.md should be both staged\n$MSG"
  exit 1
fi

as PACea/.git/hooks/pre-commit.

Then can close this.

andrew-edwards commented 4 months ago

Today I get the original error again:

README.Rmd and README.md should be both staged
use 'git commit --no-verify' to override this check

Thinking I should remove the corresponding line from the pre-commit in the previous comment.

andrew-edwards commented 4 months ago

Worked for me. Travis you will need to do the same at some point probably, i.e. remove

elif [[ ${#README[@]} -lt 2 ]]; then
  echo -e "README.Rmd and README.md should be both staged\n$MSG"
  exit 1

from .git/hooks/pre-commit. After removing those lines I could re-run the un-edited README.Rmd to automatically update README.md, but not get a git error (as with those lines in, git had been wanting .Rmd to also be staged, but it wasn't because it was not updated).

andrew-edwards commented 3 months ago

Looks to be resolved, assuming Travis did the last bit locally. Not convinced it's completely needed.