frictionless-r icon indicating copy to clipboard operation
frictionless-r copied to clipboard

Refactor `read_resource()`

Open damianooldoni opened this issue 1 year ago • 2 comments

This PR fixes #210 by refactoring read_resource(). It uses several helper functions placed in utils.R.

The PR adds the helper functions here below. Comments describe the differences between the description of @peterdesmet in #210 and the actual implementation:

  • get_encoding(). Used in create_local() (see below)
  • create_locale(). It needs resource$encoding and resource$fields. I didn't pass resource (and package) as I didn't see the advantage. It doesn't decrease the number of arguments and it makes the helper function get_encoding() longer: we need to call get_schema() in the function again.
  • create_col_types() (wrapper) and create_col_type() instead of the proposed get_cols() and get_col(). I find the verb get in the function name a little misleading: the functions are creating col types for the resource to read, not getting them from the resource.
  • col_integer(), col_number(), col_string(), col_date(), col_time(), col_datetime(), used in switch() in create_col_type(). It follows the specs in #210.
  • ~read_from_paths() (wrapper) and~ read_from_path(): using a map_df() simplify extremely the code.

We could eventually move the helper functions created in this PR in an apart utility file, maybe utils-resource.R.

No addition to NEWS.md as it doesn't affect the user experience. But we could add it if wished.

damianooldoni avatar Jul 10 '24 11:07 damianooldoni

Some errors while checking: convert to draft.

damianooldoni avatar Jul 10 '24 12:07 damianooldoni

Please create:

  • get_encoding.R
  • create_locale.R
  • create_col_types.R
  • col_types.R
  • read_from_path.R

peterdesmet avatar Jul 10 '24 13:07 peterdesmet

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (940aa7f) to head (324d49d).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #244   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        22    +4     
  Lines          597       626   +29     
=========================================
+ Hits           597       626   +29     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jul 16 '24 21:07 codecov[bot]

Well done, @peterdesmet!

damianooldoni avatar Jul 18 '24 08:07 damianooldoni