PecanProject / pecan

The Predictive Ecosystem Analyzer (PEcAn) is an integrated ecological bioinformatics toolbox.
www.pecanproject.org
Other
202 stars 231 forks source link

Updated base/qaqc #3104

Closed meetagrawal09 closed 1 year ago

meetagrawal09 commented 1 year ago

Description

The PR is aimed at fixing the "no visible binding for global variable" notes for base/qaqc

Review Time Estimate

Types of changes

Checklist:

meetagrawal09 commented 1 year ago

@mdietze, @infotroph, @Aariq if you can please help me out with the failing check 4.0

Aariq commented 1 year ago

Not sure. I tried adding rlang to Imports and it didn't help. But I did notice that the counterpart to this function, find_inputs_without_formats() probably doesn't work, and now I'm questioning whether the function you're editing even works. Might want to write some tests before you mess around with them.

meetagrawal09 commented 1 year ago

Yes, will wait for some further clarification from you on this. The only place where I see these functions being called is Link

Aariq commented 1 year ago

Yes, will wait for some further clarification from you on this. The only place where I see these functions being called is Link

https://github.com/PecanProject/pecan/blob/353b4dd2eb632f5509a4dae56b6ca388c41873d8/base/qaqc/R/find_inputs_without_formats.R#L23

You can see here that user_id_code isn't an argument for the function, so !!user_id_code will cause an error because it doesn't exist.

infotroph commented 1 year ago

@meetagrawal09 Try rerunning Roxygen — looks like the new importFrom isn’t yet showing up in NAMESPACE

meetagrawal09 commented 1 year ago

https://github.com/PecanProject/pecan/blob/353b4dd2eb632f5509a4dae56b6ca388c41873d8/base/qaqc/R/find_inputs_without_formats.R#L23

You can see here that user_id_code isn't an argument for the function, so !!user_id_code will cause an error because it doesn't exist.

Yes, I did see that issue. I believe it's a typo and should be user_id_code because the context and description for this variable in both the files find_formats_without_inputs and find_inputs_without_formats seems to be same.

meetagrawal09 commented 1 year ago

@meetagrawal09 Try rerunning Roxygen — looks like the new importFrom isn’t yet showing up in NAMESPACE

Yes that did work. If you can give some suggestion about the user_id input parameter being changed to user_id_code or the user_id_code variable used in the function body being changed to user_id pecan/base/qaqc/R/find_inputs_without_formats.R

meetagrawal09 commented 1 year ago

How can I update the Rcheck_reference.log to remove the warnings "no visible binding for global variable" ?

Also I see one issue in the logs about the below :

'::' or ':::' imports not declared from:
  ‘dplyr’ ‘PEcAn.DB’

Is there a need to add an explicit import for these ?

infotroph commented 1 year ago

How can I update the Rcheck_reference.log

The general pattern for editing Rcheck_reference.log is to delete the line that complains, and if that removes the last line of complains from a given check, edit the line for that check to end in OK instead of NOTE or WARNING.

So if you fixed all the "no visible binding" errors, then you'll edit line 47 and delete lines 48-71:

* checking foreign function calls ... OK
- * checking R code for possible problems ... NOTE
- cull_database_entries: no visible global function definition for
-   ‘read.table’
- cull_database_entries: no visible global function definition for
-   ‘write.table’
- find_formats_without_inputs: no visible binding for global variable
-   ‘user_id’
- find_formats_without_inputs: no visible binding for global variable
-   ‘created_at’
- find_formats_without_inputs: no visible binding for global variable
-   ‘updated_at’
- find_inputs_without_formats: no visible binding for global variable
-   ‘user_id_code’
- find_inputs_without_formats: no visible binding for global variable
-   ‘created_at’
- find_inputs_without_formats: no visible binding for global variable
-   ‘updated_at’
- write_out_table: no visible global function definition for
-   ‘write.table’
- Undefined global functions or variables:
-   created_at read.table updated_at
-   user_id user_id_code write.table
- Consider adding
-   importFrom("utils", "read.table", "write.table")
- to your NAMESPACE file.
+ * checking R code for possible problems ... OK
* checking Rd files ... OK

It's usually easy to see what needs deleting by hand, but if in doubt you can also rerun the package checks and paste in the output from the relevant section... but when taking this approach make sure to paste plain-text output from R CMD check, not the colorized and formatted output from devtools::check (which is nicer to read, but the script can't parse it).

After editing, confirm that the edit worked by running ./scripts/check_with_errors.R base/qaqc in your terminal -- if it complains, then one of the deleted lines isn't actually fixed yet.

Is there a need to add an explicit import for these ?

Add them to the Imports: section of DESCRIPTION. No need for any Roxygen @importFrom in this case.

meetagrawal09 commented 1 year ago

Work on this PR is now complete and it is ready for a review.