pecan icon indicating copy to clipboard operation
pecan copied to clipboard

Moved convert.input.R from utils to DB

Open nanu1605 opened this issue 2 years ago • 4 comments

To move convert.input from utils to DB

nanu1605 avatar Aug 06 '22 16:08 nanu1605

Copying the checklist we discussed in Slack here for easy reference:

  • [x] check whether convert.input uses any unexported functions from PEcAn.utils. If so, see where else they're used and decide whether to move them to data.atm, copy them, or export them from utils
  • [x] As one commit, move the convert.input function definition and man page from PEcAn.utils to PEcAn.DB.
  • [x] As a separate commit, update the convert.input code to remove PEcAn.DB:: in front of calls to DB functions and to add PEcAn.utils:: in front of calls to utils. It's important to do it in a separate commit because otherwise Git will just show one giant blob deleted from utils and another added to DB, with no easy way to find the differences between them. Doing it in two steps makes it pretty easy to verify that the move part didn't change anything inside the blob
  • [x] Review the function documentation and make any edits needed — I doubt it will, but do check
  • [ ] Implement any needed deprecation stub in PEcAn.utils. The usual best practice is to deprecate functions slowly over several releases to give users time to update, but the circular dependency demands quicker action. Decision needed: After this change, should a call to PEcAn.utils::convert.input() fail with the function not found or should it do nothing but throw a message to use the version in PEcAn.DB?
  • [x] Update the changelog and both packages’ NEWS files
  • [x] Update all calls to convert.inputs elsewhere in PEcAn
  • [x] Update Rcheck_reference.log, if needed, in all the packages touched by this PR

infotroph avatar Aug 06 '22 20:08 infotroph

This looks great! Go head and tackle the last three points in my checklist above as part of this PR and we should be able to merge it all at once.

infotroph avatar Aug 06 '22 20:08 infotroph

@infotroph To change the Rcheck_reference.log PR #3000 is already opened. Should I merge it in this PR and close that one?

nanu1605 avatar Aug 06 '22 21:08 nanu1605

@nan1605 No, just update the files that this PR touches, probably by manual editing rather than a full new check run (i.e. only change the lines that need to change because of this PR). We'll see which PR gets merged first and resolve any conflicts after that in the remaining one.

infotroph avatar Aug 06 '22 21:08 infotroph

@nanu1605 I think we'll need to add a line in both NEWS.md and changelog.md file stating that we've made a change in the function's name and why.

moki1202 avatar Aug 16 '22 12:08 moki1202

Replaced by #3026

infotroph avatar Sep 01 '22 16:09 infotroph