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

PEcAn.ED2 check warnings #3135

Merged
merged 15 commits into from
Mar 25, 2023
Merged

PEcAn.ED2 check warnings #3135

merged 15 commits into from
Mar 25, 2023

Conversation

Aariq
Copy link
Collaborator

@Aariq Aariq commented Mar 16, 2023

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
  • 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.
  • My name is in the list of CITATION.cff
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Aariq Aariq changed the title Ed check warnings PEcAn.ED2 check warnings Mar 16, 2023
Copy link
Member

@mdietze mdietze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Aariq
Copy link
Collaborator Author

Aariq commented Mar 16, 2023

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

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

@Aariq
Copy link
Collaborator Author

Aariq commented Mar 16, 2023

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

mdietze commented Mar 16, 2023

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

@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
Copy link
Collaborator Author

Aariq commented Mar 17, 2023

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

Copy link
Member

@infotroph infotroph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"some of the low-hanging", @Aariq says, as he knocks out 150 lines of warnings in one go 😄

models/ed/tests/Rcheck_reference.log Outdated Show resolved Hide resolved
models/ed/R/write.configs.ed.R Show resolved Hide resolved
models/ed/tests/Rcheck_reference.log Outdated Show resolved Hide resolved
@mdietze mdietze enabled auto-merge March 21, 2023 13:26
@mdietze mdietze added this pull request to the merge queue Mar 21, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 21, 2023
@Aariq Aariq added the Status: Ready Pull request ready for merge, or issue ready to be closed label Mar 23, 2023
@mdietze mdietze added this pull request to the merge queue Mar 24, 2023
Merged via the queue into develop with commit 54b7852 Mar 25, 2023
@Aariq Aariq deleted the ed-check-warnings branch March 27, 2023 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Ready Pull request ready for merge, or issue ready to be closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants