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

Remove hmisc dependency #102

Merged
merged 4 commits into from
Apr 12, 2024
Merged

Remove hmisc dependency #102

merged 4 commits into from
Apr 12, 2024

Conversation

lshandross
Copy link
Contributor

Solves Issue #55

Copy link

github-actions bot commented Apr 4, 2024

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 42.63566% with 74 lines in your changes are missing coverage. Please review.

Project coverage is 66.54%. Comparing base (7e12730) to head (dd86a19).
Report is 3 commits behind head on main.

❗ Current head dd86a19 differs from pull request most recent head 0316a2d. Consider uploading reports for the commit 0316a2d to get more accurate results

Files Patch % Lines
R/Hmisc-utils.R 42.18% 74 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #102       +/-   ##
===========================================
- Coverage   87.83%   66.54%   -21.30%     
===========================================
  Files           5        6        +1     
  Lines         148      275      +127     
===========================================
+ Hits          130      183       +53     
- Misses         18       92       +74     

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

@lshandross
Copy link
Contributor Author

@annakrystalli is it ok that the functions from Hmisc aren't getting covered by tests, as they've been tested in the original package?

Copy link
Contributor

@elray1 elray1 left a comment

Choose a reason for hiding this comment

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

Let's address the linting issues that are introduced by changes in this PR before merging it in.

I also just filed a separate issue, #103 , to fix the other linting issues, which are in existing stuff in the package -- my vote is that it'd be good to take care of that issue next so that we get to a place where linting checks pass. Ideally, it'd really be nice to do that other one, get that merged in, and then merge main into this branch so that the linting check fully passes. But I'm personally ok with merging a PR where the lint check is failing as long as it doesn't add new linting issues.

@lshandross
Copy link
Contributor Author

Linting issues have been fixed with pull request #105!

@lshandross lshandross requested a review from elray1 April 11, 2024 19:03
@elray1 elray1 merged commit 7358431 into main Apr 12, 2024
8 checks passed
@elray1 elray1 deleted the remove-hmisc_dependency branch April 12, 2024 17:39
@lshandross lshandross linked an issue Jul 23, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

remove Hmisc dependency
2 participants