Adds user-created parameters in fread's yaml

[eliocamp]

Part of #3540

[codecov]

Codecov Report

Merging #4123 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4123   +/-   ##
=======================================
  Coverage   99.60%   99.60%           
=======================================
  Files          72       72           
  Lines       13918    13918           
=======================================
  Hits        13863    13863           
  Misses         55       55           

---

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

[MichaelChirico]

(open-ended) is there any extra checks we should do on the data.table side here of the user input? or will it be fine to leave it up to yaml::as.yaml to do all validation?

[eliocamp]

That was my thought, yes.

[MichaelChirico]

Thanks! Looks good overall.

What do you think of an API like this?

# @export
generate_yaml = function(x) # standard YAML we generate now for object x

my_yaml = c(generate_yaml(x), list(something_else = 'o hai!'))

A bit more cumbersome, but offers the flexibility to fully customize the YAML header (potentially excluding/editing some key-values we put in there, e.g.).

PS please edit the man page accordingly and add a NEWS item

[mattdowle]

LGTMT. Great. Can see some outstanding (e.g. news item and man page) so I added the WIP tag. When you remove the WIP tag, and @MichaelChirico has approved with green tick, I'll merge.
You're already a project member so you can create branches directly in the main project. Makes it easier to commit to each other's PRs. I don't see you in DESCRIPTION though as a contributor; please add yourself there in this PR. Your previous PR (#3685) did touch a .R file but only a warning message so at the time I won't have considered that a code change. Whereas this new PR does contain code/logic changes.

[jangorecki]

those new tests has to be escaped, they uses yaml packages which is a suggested dependency

[eliocamp]

What do you think of an API like this?

# @export
generate_yaml = function(x) # standard YAML we generate now for object x

my_yaml = c(generate_yaml(x), list(something_else = 'o hai!'))

A bit more cumbersome, but offers the flexibility to fully customize the YAML header (potentially excluding/editing some key-values we put in there, e.g.).

I like the flexibility with this, but, yeah, it's very verbose. Also I don't like the idea of having to repeat the name of the object inside frwite. It opens up the very real possibility of annoying bugs if the user makes a typo or something like that (I wouldn't like to have to debug something like this fwrite(some_data, yaml = generate_yaml(som_data))).

I like TRUE as a shortcut for "just create the standard yaml that data.table creates", so how about this:

fwrite(x, yaml = list(TRUE, and_now = "something completely different"))

I'm also toying around the possibility of making it so that the yaml parameter accepts a function that takes a data.table and returns a list (that then will be parsed with as.yaml).

Then, you could write:

fwrite(x, yaml = my_yaml_function)

for maximum flexibility and portability.

[eliocamp]

I've added some logic and tests for the proposed interface.

[jangorecki]

@eliocamp are you still planning to address the feedback provided here? so we can remove WIP label and have it ready to merge.

[eliocamp]

Sorry, had forgotten about this PR. Thanks for the reminder.

I think I addressed all the feeback. What's left is maybe expanding the functionality as detailed on my previous comment: #4123 (comment)

Although maybe that's best left for another PR? Let me know if you think anything's missing.

[jangorecki]

Yes, functionality is there, but we should also have entry in news, description, etc. @mattdowle commented pointed out that well #4123 (comment)

[eliocamp]

Ok, there it is. I had to change Test numbers too.

[MichaelChirico]

@eliocamp is this worth reviving? I don't have a sense of how much uptake there has been of fwrite(yaml=TRUE). WDYT? There are no 👍 from external users on #3540 after ~6 years.

[eliocamp]

Woah, all the memories!

Yeah, I don't think lots of people are clamoring for this to be implemented. I would be fine with dropping this PR.