Fix FIMSFrame

[kellijohnson-NOAA]

What is the feature?

• Fix [Bug]: type.convert() is reformatting date to character without leading zeros in data_mile1 #639 where the leading zeros of the year if using fake years of single-digit integers are removed when saving to a csv file and reading the data back in. Instead of worrying about always having leading zeros, I have created a feature that ensures FIMSFrame creates date objects.
• This problem I think is being brought up by some change that is external to FIMS. That is, data_mile1 use to have leading zeros and now it does not but the code to create the data_mile1 has not changed in a way that would cause any differences. I think something within base might have changed. It is okay though because now FIMS will be more robust to user errors and differences in R versions.

How have you implemented the solution?

Using as.Date() for datestart and dateend to ensure the leading zeros are present. And, the format portion of as.Date() ensures that the dates are formatted with dashes instead of slashes and will give users an error if they are not.

Does the PR impact any other area of the project?

Objects from FIMSFrame are not really being used in the code right now so it is not a well-spread problem. We just noticed it in the data group when we started using the slots of FIMSFrame for things.

How to test this change

The change is tested in the automated tests.

Developer pre-PR checklist

• I relied on GitHub actions to 🧪 things for me while I sat on the 🛋️.
• Ran some of the R tests locally and also used this branch to rebase the m2-r-output branch to see if the broken tests there were finally passing and they are 🥳

This PR is broken up into two commits (that should later be squashed) but I would like comments from @k-doering-NOAA on the first commit and comments from @iantaylor-NOAA on the second commit. Thank you.

Closes #639

[github-actions]

Instructions for code reviewer

Hello reviewer, thanks for taking the time to review this PR!

• Please use this checklist during your review, checking off items that you have verified are complete!
• For PRs that don't make changes to code (e.g., changes to README.md or Github actions workflows), feel free to skip over items on the checklist that are not relevant. Remember it is still important to do a thorough review.
• Then, comment on the pull request with your review indicating where you have questions or changes need to be made before merging.
• Remember to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do.
• PR reviews are a great way to learn, so feel free to share your tips and tricks. However, for optional changes (i.e., not required for merging), please include nit: (for nitpicking) before making the suggestion. For example, nit: I prefer using a data.frame() instead of a matrix because...
• Engage with the developer when they respond to comments and check off additional boxes as they become complete so the PR can be merged in when all the tasks are fulfilled. Make it clear when this has been reached by commenting on the PR with something like This PR is now ready to be merged, no changes needed.

Checklist

• The PR is requested to be merged into the appropriate branch (typically main)
• The code is well-designed.
• The functionality is good for the users of the code.
• Any User Interface changes are sensible and look good.
• The code isn’t more complex than it needs to be.
• Code coverage remains high, indicating the new code is tested.
• The developer used clear names for everything.
• Comments are clear and useful, and mostly explain why instead of what.
• Code is appropriately documented (doxygen and roxygen).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.88%. Comparing base (8aacee0) to head (624714f).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #641      +/-   ##
==========================================
+ Coverage   78.56%   78.88%   +0.32%     
==========================================
  Files          36       36              
  Lines        1945     1975      +30     
  Branches      141      141              
==========================================
+ Hits         1528     1558      +30     
  Misses        374      374              
  Partials       43       43              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[iantaylor-NOAA]

Nice work @kellijohnson-NOAA.

I like the new validate_data_colnames() function, and the extra format checks within FIMSFrame(). It's probably useful that the bug cropped up because this system of error checks will surely pay off in the future by being more robust (as you noted in the PR).

[k-doering-NOAA]

The column names of the S4@data object were being checked with the
validation function but there was no check in FIMSFrame and to do the
calculations within FIMSFrame all of the columns need to be present.
Right now we are checking it twice, once in FIMSFrame and once in the
validator of the FIMSFrame class. We might need a different class like an
input data class and we could check the class upon input to FIMSFrame but
this works for right now. @k-doering-NOAA do you have any ideas here?

From reading on the validate function in the advanced R book, it sounds like checking should be done in the validation function; however, if it means the user will not get a helpful error, that does seem like a problem. Maybe we should be a little unconventional (I think) and just do the checking when putting together the dataframe? Or is there a reason we should be checking the columns names twice?

Just an idea, though, and I think your solution is fine for now!

[kellijohnson-NOAA]

Thanks @iantaylor-NOAA and @k-doering-NOAA for the review and comments. I have been thinking about the best way forward (hence the delay in my response) and I think we should leave the check in there twice for right now because we have multiple developers working on the methods and checking in the validity function for the column names of the output to be correct saves us from writing an additional formal test in the testthat code. So, in the future we might need to remove it but the extra precaution it provides right now appears to be a good thing.