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

Improving test coverage, doi badge, fix read_fhx blank space bug #162

Merged
merged 6 commits into from
Jul 3, 2020

Conversation

chguiterman
Copy link
Contributor

@chguiterman chguiterman commented Jun 22, 2020

Also includes:

  • A minor fix to read_fhx() for flexibility
  • A minor edit recommended R CMD check
  • Add DOI badge to README

@coveralls
Copy link

coveralls commented Jun 22, 2020

Coverage Status

Coverage increased (+16.6%) to 92.549% when pulling b18414a on chguiterman:master into 0cb419d on ltrr-arizona-edu:master.

@brews
Copy link
Member

brews commented Jun 22, 2020

Hey @chguiterman! Thanks for the code!

Are you ready for a review on this PR?

Could you walk me through your "A minor fix to read_fhx() for flexibility"... How did you come across this? What happens, exactly?

@chguiterman
Copy link
Contributor Author

Hey @brews
Yes, please review and merge if there's nothing to further correct.
The "minor fix to read_fhx()" was to tell burnr to ignore blank spaces at the end of an FHX file. I encountered some read-in errors upon loading IMPD files that this fixes. The added code was inserted in line 126 of "io.R" where we drop the dots (".") from the series. Sometimes there's a blank line after the last ring, and "\032" is the ASCII code for a blank.

@brews brews self-assigned this Jul 3, 2020
@brews brews added bug Something isn't working enhancement New feature or request labels Jul 3, 2020
@brews brews changed the title Improving coverage in unit testing Improving test coverage, doi badge, fix read_fhx blank space bug Jul 3, 2020
@brews brews merged commit cbba0dc into ltrr-arizona-edu:master Jul 3, 2020
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.

These changes generally look good. Really happy to see coverage improve, @chguiterman! Thanks so much for this PR.

Given the time crunch I'm fine to merge this but I created new issues to

These need to be taken care of before our next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants