PEcAn.ED2 check warnings

PecanProject / pecan

The Predictive Ecosystem Analyzer (PEcAn) is an integrated ecological bioinformatics toolbox.

www.pecanproject.org

Other

202 stars 235 forks source link

PEcAn.ED2 check warnings #3135

Closed Aariq closed 1 year ago

Aariq commented 1 year ago

Description

Just fixing some of the lower-hanging fruit in terms of R CMD check warnings and notes for PEcAn.ED2

Motivation and Context

Review Time Estimate

[ ] Immediately
[ ] Within one week
[x] When possible

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[ ] My change requires a change to the documentation.
[x] My name is in the list of CITATION.cff
[ ] I have updated the CHANGELOG.md.
[x] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.

Aariq commented 1 year ago

Looks good. The only things I see missing are a note in the changelog and an updated Rcheck_reference.log to enforce that these errors can't creep back in.

Are there instructions somewhere on how to update the Rcheck_reference.log? I couldn't find anything in the documentation or in CONTRIBUTING.md

meetagrawal09 commented 1 year ago

@Aariq, may be this comment can help you with updating Rcheck_reference.log

Aariq commented 1 year ago

@Aariq, may be this comment can help you with updating Rcheck_reference.log

Oh wow, that's a lot more work than I expected. Won't have time for this for a while. Thanks for pointing me in the right direction though!

mdietze commented 1 year ago

@Aariq rather than making line-by-line edits to the log, if you make a lot of changes it can often be easier to just replace the existing log with a copy-and-paste of the entire plain-text output from R CMD check. If done correctly the resulting PR diff should end up highlighting just the things you fixed.

@infotroph at some point it would be good to summarize your guidance about log updates into the PEcAn documentation itself

infotroph commented 1 year ago

@Aariq Alternate approach: Follow the instructions in lines 84-95 of scripts/check_with_errors.R. The result is the same as the copy-paste approach Mike suggested, but with a smidge less room for configuration differences to creep in because you know you're using e.g. the same check flags as the replaced file was.

Aariq commented 1 year ago

Ok, I took your suggestion @infotroph and committed the new log