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

Shifted convert.input.R from utils to DB #3026

Merged
merged 17 commits into from
Sep 22, 2022

Conversation

nanu1605
Copy link
Collaborator

@nanu1605 nanu1605 commented Aug 31, 2022

To move convert.input from utils to DB

This replaces #3009, which accumulated enough conflicts that it was easier to redo the changes in a new branch.

@nanu1605
Copy link
Collaborator Author

nanu1605 commented Aug 31, 2022

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

  • 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
  • As one commit, move the convert.input function definition and man page from PEcAn.utils to PEcAn.DB.
  • 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
  • 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?
  • Update the changelog and both packages’ NEWS files
  • Update all calls to convert.inputs elsewhere in PEcAn
  • Update Rcheck_reference.log, if needed, in all the packages touched by this PR

@nanu1605 nanu1605 added the Status: Ready Pull request ready for merge, or issue ready to be closed label Aug 31, 2022
@@ -0,0 +1,891 @@
##' Convert between formats, reusing existing files where possible
Copy link
Member

Choose a reason for hiding this comment

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

this looks like an existing file that should have been moved via git mv so as to retain version history

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mdietze when I try to do it using git mv it says fatal: not under version control, source=base/utils/R/convert_input.R, destination=base/db/R/convert_input.R

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nanu1605 make sure you use the right case in the paths.

Copy link
Member

Choose a reason for hiding this comment

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

@mdietze Rebased to address this -- long story short it was less about mv vs add/rm and more that the old filename was retained with new content. Placing the defunct stub in a separate file allowed Git to detect the rename correctly.

@mdietze mdietze merged commit a27b596 into PecanProject:develop Sep 22, 2022
@nanu1605 nanu1605 deleted the move_convert.input branch October 9, 2022 18:48
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