Moved `robustly` function to utils

PecanProject / pecan

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

www.pecanproject.org

Other

202 stars 235 forks source link

Moved `robustly` function to utils #3096

Closed meetagrawal09 closed 1 year ago

meetagrawal09 commented 1 year ago

Description

The PR moves robustly function to utils from PEcAn.data.atmosphere This PR fixes #3073

Motivation and Context

Review Time Estimate

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

Types of changes

[X] 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.
[X] I have read the CONTRIBUTING document.
[ ] I have added tests to cover my changes.
[ ] All new and existing tests passed.

moki1202 commented 1 year ago

@meetagrawal09 Just want to check whether you manually moved the file or used git mv. I would highly recommend using the latter. Reviewers please correct me if I'm wrong here.

meetagrawal09 commented 1 year ago

@meetagrawal09 Just want to check whether you manually moved the file or used git mv. I would highly recommend using the latter. Reviewers please correct me if I'm wrong here.

@moki1202 sir which file are we talking about here ? Is it the robustly.Rd file you are talking about ? ( genetated again after moving the function using roxygen2 ) And if you meant the robustly.R file then I added that function to utils.R ( thus no movement of file really was needed )

moki1202 commented 1 year ago

Removing the function entirely from the previous package would mean a little trouble for PEcAn users. A good practice here is to leave the deprecated version of the function at its original place and update the changelog file to let the users know so they don't have any troubles finding the function. Again I'm not a 100% sure about this so I'll leave it to @Aariq and @infotroph

Aariq commented 1 year ago

It sounds like it's not a good idea to move robustly to utils. See: https://github.com/PecanProject/pecan/issues/3073#issuecomment-1405446745

infotroph commented 1 year ago

Thanks for taking initiative on this, @meetagrawal09! Let's finish the discussion in #3073 before working more on this, but please do keep the PR open until there's a formal decision -- my comment there is only one opinion, and others may have counterpoints. I've been loudly wrong many times before on this project :)

meetagrawal09 commented 1 year ago

Sure, will keep the PR open until any formal decision.