Skip to content

Add fwf_positions functionality and utilize vroom instead of readr#2

Open
jimbrig wants to merge 3 commits intoprontog:masterfrom
jimbrig:master
Open

Add fwf_positions functionality and utilize vroom instead of readr#2
jimbrig wants to merge 3 commits intoprontog:masterfrom
jimbrig:master

Conversation

@jimbrig
Copy link

@jimbrig jimbrig commented Mar 6, 2020

Related to issue #1

@prontog
Copy link
Owner

prontog commented Mar 6, 2020

@jimbrig2011 thanks for PR! It looks good.
A few questions:

  1. What about making multi.specs a named list of col_positions and leave everything to the user? The downside is that it will break existing users (most likely just me) because the API will change. What do you think?
  2. There is a warning with the if (spec.type == "widths") condition. This will not be accepted by CRAN. If you change the default value to a "widths" the warning will go away. Also it would be safer to add the optional variable after the mandatory ones to avoid breaking programs (mine?) that pass variables by position.
  3. What is vroom? Lol, I just checked it out, it seems to be the evolution of readr. It looks good. I prefer though to have a third function (lets say vroom_multi_fwf), with links to vroom in the so that its clear that it's just a wrapper function.

Again thanks for taking time to evaluate this package and for the PR. If you don't want to make any changes or disagree with my remarks, I will happily apply them myself.

Best Regards,

Panos

@jimbrig
Copy link
Author

jimbrig commented Apr 2, 2020

@prontog sorry man, been too busy to circle back - I'll look into the aforementioned issues this week. Also, vroom is a tidyverse package build on top of readr but much faster and efficient. Check it out here. The readme is pretty informative.

@prontog
Copy link
Owner

prontog commented Apr 2, 2020

@jimbrig2011 take your time.There's no hurry. It's great to hear from you again! I though I lost my second verified user :)
I'm interested in vroom and when I get some time (at the moment I've been struggling with remote work due to covid-19) I will definitely run some benchmarks against real data.

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.

2 participants