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

Add check for overlapping files #169

Merged
merged 7 commits into from Dec 7, 2020
Merged

Add check for overlapping files #169

merged 7 commits into from Dec 7, 2020

Conversation

agila5
Copy link
Collaborator

@agila5 agila5 commented Jun 5, 2020

Fixes #168. These are some tests with the "new" function:

stats19:::check_overlapping_files(1979:2018)
#> [1] 1979 2005 2015 2016 2017 2018
stats19:::check_overlapping_files(2005:2018)
#> [1] 2005 2015 2016 2017 2018
stats19:::check_overlapping_files(2009:2018)
#>  [1] 2009 2010 2011 2012 2013 2014 2015 2016 2017 2018
stats19:::check_overlapping_files(2004:2018)
#> [1] 1979 2005 2015 2016 2017 2018

Created on 2020-06-05 by the reprex package (v0.3.0)

I'm not sure if I should also add a message or a warning.

@Robinlovelace
Copy link
Member

Very nice! I plan to give it a test run tomorrow. Thank you Andrea!

@codecov-commenter
Copy link

codecov-commenter commented Jun 5, 2020

Codecov Report

Merging #169 into master will increase coverage by 0.15%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
+ Coverage   71.91%   72.06%   +0.15%     
==========================================
  Files           7        7              
  Lines         445      451       +6     
==========================================
+ Hits          320      325       +5     
- Misses        125      126       +1     
Impacted Files Coverage Δ
R/get.R 97.33% <83.33%> (-1.22%) ⬇️

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 93fe110...4e667ae. Read the comment docs.

@layik
Copy link
Member

layik commented Dec 3, 2020

Hi @agila5 apologies for this long delay. I need stats19 to function better so will try to fix this first. As you have done a neat job on this, I will leave you to finish it off. Please see my changes in the same branch mainly to indicate where the checks are needed. You decide how you proceed, or if you are busy I will probably just move your neater checks into the check_year function and then just add it to get as well as done in my commit to this branch.

Thank you,it is an annoying issue.

@layik
Copy link
Member

layik commented Dec 3, 2020

Fixing the build...

@agila5
Copy link
Collaborator Author

agila5 commented Dec 3, 2020

Hi @layik! Thanks for working again on this project, I totally forgot about this PR.

As you have done a neat job on this, I will leave you to finish it off. Please see my changes in the same branch mainly to indicate where the checks are needed. You decide how you proceed, or if you are busy I will probably just move your neater checks into the check_year function and then just add it to get as well as done in my commit to this branch.

I'm sorry, but I'm not sure what to do. Do you want me to move the tests reported in the first comment into the package's tests? In any case, feel free to adjust the function(s)!

Fixing the build...

I just checked the GHA actions' errors, and I have no idea what's going on and I can't even understand which tests are failing 😅 Do you? If not maybe we can just close this PR and create a new one starting from the current master branch.

@layik
Copy link
Member

layik commented Dec 4, 2020

@agila5 it passes on my local machine, you are right it is hard to read the logs. Leave it with me in terms of fixing the build and no do not close the PR as we have solved the issue twice over, so we will decide how to use them and then merge.

Bear with me please.

@layik
Copy link
Member

layik commented Dec 5, 2020

@agila5 now have a look check_year and your function and see if former is enough. After that should be good to be merged.

@agila5
Copy link
Collaborator Author

agila5 commented Dec 6, 2020

Hi @layik! IMO there is a small problem with 1979 - 2004 range:

# update pkgs
remotes::install_github("ropensci/stats19", ref = "fix_multiple_years")
#> Using github PAT from envvar GITHUB_PAT
#> Skipping install of 'stats19' from a github remote, the SHA1 (7a63b0cb) has not changed since last install.
#>   Use `force = TRUE` to force installation

# tests
stats19:::check_year(1979:2018)
#> Year not in range, changing to match 2005:2014 data
#>  [1] 1979 1980 1981 1982 1983 1984 1985 1986 1987 1988 1989 1990 1991 1992 1993
#> [16] 1994 1995 1996 1997 1998 1999 2000 2001 2002 2003 2004 2005 2015 2016 2017
#> [31] 2018

Created on 2020-12-06 by the reprex package (v0.3.0)

I think it should return c(1979, 2005, 2015, ..., 2018) since I think 1979 -> 2004 data are all aggregated into a unique file, right? I would use the same approach as 2005:2014 (i.e. test + unique).

Apart from that, I think you can use check_year() and delete my function!

@layik
Copy link
Member

layik commented Dec 7, 2020

@agila5 I am ready to merge this if you are happy. Many thanks for the effort here makes my current work much easier.

@Robinlovelace
Copy link
Member

LGTM 👍 just need to remove the .travis.yml file and builds should be fine on master. Good to go from my perspective 🚀

@Robinlovelace Robinlovelace merged commit 1c08e39 into master Dec 7, 2020
@Robinlovelace Robinlovelace deleted the fix_multiple_years branch December 7, 2020 11:40
@agila5
Copy link
Collaborator Author

agila5 commented Dec 7, 2020

Agree, happy to merge.

@Robinlovelace
Copy link
Member

Now on Master. Worth adding an element to the news.md file @agila5 ? Not essential but good practice.

@agila5
Copy link
Collaborator Author

agila5 commented Dec 7, 2020

Done!

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.

Minor issue with get_stats19 and multiple years
4 participants