pecan icon indicating copy to clipboard operation
pecan copied to clipboard

Fix check warnings in photosynthesis (replaces #2821)

Open infotroph opened this issue 3 years ago • 2 comments

This PR initially contains the same commits as #2821, which can no longer be merged because its upstream fork was deleted. Please see there for a lot of good discussion, not all of which was resolved yet when I opened this PR on 2022-08-17.

Goal of any additional commits: Make the minimal changes to get @moki1202's good work merged; it is OK if not every warning in the photosynthesis package is cleaned up.

infotroph avatar Aug 18 '22 03:08 infotroph

@infotroph what would be the git commands if I wanted to push changes here?

moki1202 avatar Aug 23 '22 07:08 moki1202

what would be the git commands if I wanted to push changes here?

@moki1202 two options:

a. Push directly to this branch. I think you have permission to do this already; if not I can find out how to grant it.

  • add my fork as a remote if you haven't already: git remote add infotroph https://github.com/infotroph/pecan.git
  • check out a local branch that tracks mine: git fetch infotroph && git switch -c moki-patch-16 (I should have given the branch a more meaningful name, but now that the PR is open GitHub won't let me rename it)
  • check out a new branch to work from locally: git switch -c photosynthesis-check-warnings
  • commit changes in the photosynthesis-check-warnings branch
  • when ready to push,
    • switch back to the branch that tracks this PR: git switch moki-patch-16
    • make sure you have the latest remote changes: git pull infotroph moki-patch-16
    • merge in your changes: git merge photosynthesis-check-warnings
    • push back: git push infotroph moki-patch-16
  • Pro: Simple, adds to the PR immediately without needing to wait for me to do anything.
  • Con: Needs more care to make sure we both keep remote changes pulled into our local branches.

b. Push to your fork and contribute to this branch via PR. Steps that differ from option a are in bold.

  • add my fork as a remote if you haven't already: git remote add infotroph https://github.com/infotroph/pecan.git
  • check out a local branch that tracks mine: git fetch infotroph && git switch -c moki-patch-16 (I should have given the branch a more meaningful name, but now that the PR is open GitHub won't let me rename it)
  • check out a new branch to work from locally: git switch -c photosynthesis-check-warnings
  • commit changes in the photosynthesis-check-warnings branch
  • push changes to your fork as you go: git push origin photosynthesis-check-warnings (this assumes you have an origin remote that points to github.com/moki1202/pecan.git; if it's named something other than origin then use that name)
  • when you're ready to contribute your changes, open https://github.com/infotroph/pecan/compare/moki-patch-16...moki1202:pecan:photosynthesis-check-warnings in your browser. This will take you directly to the page for opening a pull request from your branch (moki1202:photosynthesis-check-warnings) into mine (infotroph:moki-patch-16). Click "Create pull request", add a title and description, and click "create pull request" again. (There's a way to get to this page through a series of UI actions too, but they're tedious to write out and the URL works well once you know that the pattern is github.com/[target owner]/[target repo]/compare/[target branch]...[source owner]:[source repo]:[source branch].)
  • Optional but useful: Ping me here to make sure I saw the PR -- I have a history of missing the notifications for things that happen in my fork rather than in the PEcAnProject upstream.
  • Pro: Less care needed to keep branches in sync, alows easy review and discussion as we add things
  • Con: Your changes won't show up in this PR, the one we actually care about, until I remember to review and merge the PR you created.

infotroph avatar Aug 24 '22 06:08 infotroph

@infotroph what would be the git commands if I wanted to push changes here?

A lot of this is automated by the usethis package. I think you can do usethis::pr_fetch(3015) and it does everything in option a that @infotroph suggested. You'll just need to make commits and push changes. Screenshot 2022-12-13 at 6 23 34 PM

Aariq avatar Dec 13 '22 23:12 Aariq

@mdietze and @moki1202: Since your last review I added a pile of edits to the plotting code in the vignette. I had to shrink the figures a lot to get its filesize below the magic 5-MB threshold where R CMD check stops complaining -- can you please build the updated vignette locally and confirm it looks acceptable to you?

infotroph avatar Jan 30 '23 12:01 infotroph