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

Switch from lubridate to hms #28

Merged
merged 7 commits into from
Oct 2, 2018
Merged

Switch from lubridate to hms #28

merged 7 commits into from
Oct 2, 2018

Conversation

polettif
Copy link
Contributor

@polettif polettif commented Oct 1, 2018

Sorry for the kinda erratic development but I had to revisit #26. Apparently tidyverse functions don't work with lubridate (see github issue). That's why I changed the implementation of set_hms_times to work with hms::hms. That way, joining and otherwise filtering tables seems to work. The time functions of lubridate would've been a bit overkill anyways, since we don't have to work with timezones and so on.

Maybe more important: There's now tests covering the two methods added in #26. I also fixed a bug with date ranges.

@codecov-io
Copy link

codecov-io commented Oct 1, 2018

Codecov Report

Merging #28 into master will increase coverage by 5.49%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #28      +/-   ##
=========================================
+ Coverage   50.61%   56.1%   +5.49%     
=========================================
  Files          11      11              
  Lines         735     745      +10     
=========================================
+ Hits          372     418      +46     
+ Misses        363     327      -36
Impacted Files Coverage Δ
R/time.R 96.55% <100%> (+75.71%) ⬆️

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 d6ea344...cc3bb7f. Read the comment docs.

@polettif
Copy link
Contributor Author

polettif commented Oct 1, 2018

Fixed the failing tests.

@tbuckl I saw you made set_hms_times internal. What's your proposed workflow to use it? Because if it's not exported we should include in read_gtfs?

@tbuckl
Copy link
Member

tbuckl commented Oct 2, 2018

first of all, thanks so much for the tests. everyone can appreciate tests.

regarding making set_hms_times internal, the improvements you had made this month to read_gtfs seemed so useful to the functioning and clarity of the entire package that i wanted to make sure to get it into CRAN before i had my head wrapped around set_hms_times. it looks like a useful function i just want to be able to write a small example before including it. as you say, i think its ideal usage is in read_gtfs, perhaps as an optional argument (e.g. parse_time=TRUE)

@tbuckl tbuckl merged commit ed3424c into r-transit:master Oct 2, 2018
@tbuckl
Copy link
Member

tbuckl commented Oct 2, 2018

also, good catch with lubridate/hms!

@polettif
Copy link
Contributor Author

polettif commented Oct 2, 2018

regarding making set_hms_times internal, the improvements you had made this month to read_gtfs seemed so useful to the functioning and clarity of the entire package that i wanted to make sure to get it into CRAN before i had my head wrapped around set_hms_times. it looks like a useful function i just want to be able to write a small example before including it. as you say, i think its ideal usage is in read_gtfs, perhaps as an optional argument (e.g. parse_time=TRUE)

No worries, there's no need to rush things

@polettif polettif deleted the fix-dates branch October 2, 2018 10:58
@polettif polettif restored the fix-dates branch October 30, 2018 17:59
@polettif polettif mentioned this pull request Jan 28, 2019
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.

None yet

3 participants