Closed gowthamrao closed 4 years ago
Merging #30 into develop will decrease coverage by
0.08%
. The diff coverage is0%
.
@@ Coverage Diff @@
## develop #30 +/- ##
==========================================
- Coverage 6.25% 6.16% -0.09%
==========================================
Files 4 5 +1
Lines 448 454 +6
==========================================
Hits 28 28
- Misses 420 426 +6
Impacted Files | Coverage Δ | |
---|---|---|
R/WebApi.R | 62.22% <ø> (+10.37%) |
:arrow_up: |
R/private.R | 0% <0%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 71ee052...5285625. Read the comment docs.
A few comments:
It seems you have rewritten .checkBaseUrl function while maintaining its functionality. What I think you are going for is better readability of the code, but the price to pay is a dependency on the rex package. I think I prefer not having the dependency.
Have you made sure you can build the package? Does it pass R check? The reason I ask is that is still see %>%
, which I don't think would work without having magrittr on the search path.
This might be a good opportunity to add a few unit tests that make sure this function works. You could test whether it detects some example malformed and well-formed URLs (see https://github.com/OHDSI/ROhdsiWebApi/blob/develop/tests/testthat/test-webapi.R)
@gowthamrao I would agree with @schuemie on the first comment especially. Are there cases in which the original function does not work correctly?
based on vignette for rex package here https://cran.r-project.org/web/packages/rex/vignettes/url_parsing.html
rex package https://cran.r-project.org/web/packages/rex/index.html
updated roxygen documentation and description file
discussion to support this change here https://github.com/OHDSI/ROhdsiWebApi/pull/28#discussion_r364924527