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

Update unit test for write_fhx() #166

Merged
merged 3 commits into from
Jul 10, 2020
Merged

Conversation

chguiterman
Copy link
Contributor

@chguiterman chguiterman commented Jul 7, 2020

Continuation of PR#162.

I found that the FHX files created by write_fhx() are slightly different from "standard" FHX2 files. The difference is a space between the data block and years column. I added this space to the output from list_filestrings() because that function is only called by write_fhx(). The year vector now includes a space, which more correctly formats the output FHX files.

This addition facilitated an expect_equal() unit test for write_fhx() that was previously being skipped.

The newly formatted FHX file looked OK and read in fine with read_fhx().

Close #163.
Close #164.

(1)`reshape2` is "retired" withthe recomendation to transition to `tidyr`. Updated dcast to pivot_wider, added @importFrom to function documentation, which passes to the NAMESPACE and required Imports in DESCRIPTION.
(2) 'Standard' FHX2 files include a space between the data block and year. `write_fhx()` did not include that space. This did not cause problems for `read_fhx()` or any other outside program, but it made it difficult to error check the output of `write_fhx()`. `list_filestrongs()` now adds the space. My checks on the created FHX files all look fine.

* test-io.R: Now checks for equal creation of an FHX file. And the years output from `list_filestrings()` is updated to the character vector.
@coveralls
Copy link

coveralls commented Jul 7, 2020

Coverage Status

Coverage increased (+0.04%) to 92.588% when pulling 9e9def3 on chguiterman:master into cbba0dc on ltrr-arizona-edu:master.

Copy link
Member

@brews brews left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks so much for the update, @chguiterman.

One requested change before merging:

  • Could you add a new bullet point in NEWS mentioning the new packages we depend on? If we require people to install new packages, I'd like that explicitly listed.

Thanks again! This is good stuff!

@chguiterman
Copy link
Contributor Author

Good idea @brews -- got it.

Copy link
Member

@brews brews left a comment

Choose a reason for hiding this comment

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

I think this looks good! Thanks for the update. I'll merge away.

I'm updating also your main PR description to close the relevant issues.

@brews brews merged commit fa3c035 into ltrr-arizona-edu:master Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add read_fhx() unit test for whitespace at end of file Update NEWS.md
3 participants