v0.4.10: code review - Githubissues

jesse-ross commented 3 months ago

This looks good overall! I think the only essential change, from my perspective, is fixing the outdated linter config.

Linter config

The linter configuration should be updated. As of lintr 3.0.0, camel_case_linter has been removed (it was deprecated back in 2017).

Something like this should work:

linters: linters_with_defaults(
  object_name_linter = NULL,
  line_length_linter(120),
  cyclocomp_linter(25)
  )
exclusions: list("tests/testthat/data/prep_test_data.R")

Additionally, there are two files which have # nolint start but do not have a corresponding # nolint end. This is now an error. Either the closing tag should be added, or if the whole files are to be ignored they could be moved to the exclusions portion of the .lintr. The files in question are inst/extdata/gage_01013500_data.R and inst/extdata/geometry_data.R.

Lintr warnings

There are lots of linter messages. Most of them are low-priority, and they can visually swamp the more serious warnings. It might be worth configuring the linter to ignore the ones you don't care about.

Some of the ones at the "warning" level would be nice to fix, as they can lead to bugs.

Specifically I'd recommend fixing these two:

seq_linter: These come up when you iterate from 1 to the length of a vector or the height of a data frame. This can lead to errors when the vector is of length 0. The linter recommends nice replacement functions like seq_len and seq_along, which are robust to zero-length inputs.
vector_logic_linter: This comes up when you have things like this:
```
while(d <= distance & !is.na(local_dir)) {
```
In cases like these, where you're comparing to length-one booleans, switching to && is slightly more efficient and more readable.

dblodgett-usgs commented 3 months ago

Thank you Jesse! Definitely some out of date code in there that could use some clean up. I'll touch up a few things you noted to make sure there aren't any horribly broken windows and go ahead and ship it.

dblodgett-usgs commented 2 months ago

Looking this over more carefully, given the overall lifecycle of this code, I'm not going to do any code clean up. The warnings emmited in tests are all expected from changes in dependency packages and the lintr issues, while not ideal, aren't big enough issues to prompt an update to what is essentially an end of life package. The goal here is to publish this work at the current (working) state and move on to the next phase of it. These clean up/improvement fixes will be taken care of in that future work.

I did fix the .lintr format and added the two #nolint end issues. Thanks again for your time Jesse!

jesse-ross commented 2 months ago

Looking this over more carefully, given the overall lifecycle of this code, I'm not going to do any code clean up. The warnings emmited in tests are all expected from changes in dependency packages and the lintr issues, while not ideal, aren't big enough issues to prompt an update to what is essentially an end of life package. The goal here is to publish this work at the current (working) state and move on to the next phase of it. These clean up/improvement fixes will be taken care of in that future work.

I did fix the .lintr format and added the two #nolint end issues. Thanks again for your time Jesse!

That sounds just fine! Glad I could help move this along.

dblodgett-usgs commented 2 months ago

Release is cut here https://code.usgs.gov/wma/nhgf/reference-fabric/hyrefactor/-/releases/v0.4.11

DOI-USGS / hyRefactor

v0.4.10: code review #57

Linter config

Lintr warnings