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

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 opened this issue Nov 2, 2023 · 12 comments
Assignees
Labels
for Version 1.0.0.0 Needs completing before official release

Comments

@andrew-edwards
Copy link
Member

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 andrew-edwards added the for Version 1.0.0.0 Needs completing before official release label Nov 2, 2023
@andrew-edwards andrew-edwards self-assigned this Nov 2, 2023
@andrew-edwards
Copy link
Member Author

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

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 added a commit that referenced this issue Nov 3, 2023
…works (i.e. can commit newer README.Rmd than README.Md).
@andrew-edwards
Copy link
Member Author

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

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

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

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

andrew-edwards commented Nov 3, 2023

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 added a commit that referenced this issue Nov 3, 2023
…ME-northhecatetemp-1.png) and check still builds fine on README on GitHub, #44.
@andrew-edwards
Copy link
Member Author

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

@andrew-edwards
Copy link
Member Author

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

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 added a commit that referenced this issue Feb 21, 2024
…EWS.md; had to remove four lines from pre-commit hook to not get git error again (#44, not pushed though).
@andrew-edwards
Copy link
Member Author

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for Version 1.0.0.0 Needs completing before official release
Projects
None yet
Development

No branches or pull requests

3 participants