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 basic functionality of get_records() #12

Merged
merged 11 commits into from
Nov 14, 2023
Merged

Conversation

PietrH
Copy link
Member

@PietrH PietrH commented Nov 13, 2023

get_records() is now in a minimally functional state.

What works:

  • You can get all records for a custom inspection (i.e. Vespa-Watch
  • All columns get the same labels as they have in the interface
  • These labels are converted into r friendly equivalents, so underscores instead of spaces
  • The package will retry 3 times if the request fails

What is missing:

  • Any filtering like 2. Valideren
  • The function is completely uncovered by unit tests
  • All columns are imported as character strings, we have column class information from get_fields()
  • We could convert some values to their R equivalent, for example: toestemming_gegevensgebruik == "on" might make more sense as toestemming_gegevensgebruik == TRUE, we could use date and time fields for datum_tijd_registratie and datum_observatie to save users on having to to these conversions themselves.
  • List columns are still present in the output
  • There are no elegant failure states for a request that goes wrong in any way
  • The function takes a while to run, but has no progress reporting at all

Questions

  • Howmany records are we expecting? I'm getting around 17994 at the moment. Is that in the right ballpark?
  • We are expecting some values to me missing, some even most of the time. But I don't know what is expected to be mostly filled in and what is not. Does this look about right?

https://drive.google.com/file/d/1a8bVYH_SYoZklF8xKLRBixdFGvlfgGh-

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (56a0bb0) 0.00% compared to head (08622e8) 0.00%.

❗ Current head 08622e8 differs from pull request most recent head 1479cb2. Consider uploading reports for the commit 1479cb2 to get more accurate results

Additional details and impacted files
@@          Coverage Diff          @@
##            main     #12   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          3       3           
  Lines         47      61   +14     
=====================================
- Misses        47      61   +14     
Files Coverage Δ
R/get_records.R 0.00% <0.00%> (ø)

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

@PietrH PietrH self-assigned this Nov 13, 2023
@PietrH PietrH added the enhancement New feature or request label Nov 13, 2023
@PietrH
Copy link
Member Author

PietrH commented Nov 13, 2023

An issue remains where only one image url seems to be fetched for all records

@PietrH
Copy link
Member Author

PietrH commented Nov 13, 2023

An issue remains where only one image url seems to be fetched for all records

False alarm, I was looking at foto_bestrijding which only has a value for a single record.

@PietrH
Copy link
Member Author

PietrH commented Nov 13, 2023

@SanderDevisscher Is there any must have functionality missing from get_records(), or can we go ahead and merge into main and work on the other features independently?

Feel free to try it out on this branch with: get_records(access_token = get_access_token(username = "your_username"))

You'll need to install the package first, you can do this by hitting the Install button, under Build in RStudio. This should be visible if you load the project included in this repo. Let me know if you need any help! 😉

1 similar comment
@PietrH
Copy link
Member Author

PietrH commented Nov 13, 2023

@SanderDevisscher Is there any must have functionality missing from get_records(), or can we go ahead and merge into main and work on the other features independently?

Feel free to try it out on this branch with: get_records(access_token = get_access_token(username = "your_username"))

You'll need to install the package first, you can do this by hitting the Install button, under Build in RStudio. This should be visible if you load the project included in this repo. Let me know if you need any help! 😉

@SanderDevisscher
Copy link
Collaborator

Still in draft, should I wait ?

@PietrH PietrH marked this pull request as ready for review November 13, 2023 15:48
@PietrH
Copy link
Member Author

PietrH commented Nov 13, 2023

Sorry, no go ahead

Copy link
Collaborator

@SanderDevisscher SanderDevisscher left a comment

Choose a reason for hiding this comment

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

Function works as wanted.

@SanderDevisscher SanderDevisscher merged commit 1c4c0d3 into main Nov 14, 2023
6 checks passed
@SanderDevisscher SanderDevisscher deleted the get-records branch November 14, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants