bcpeinhardt / simplifile

Simple file operations for Gleam that work on all targets (Erlang/Node/Deno)
71 stars 10 forks source link

Consider swapping the arguments of the write functions #16

Closed lpil closed 9 months ago

lpil commented 9 months ago

Hello!

Hayleigh mentioned that she tends to get the order of the arguments of the file write functions wrong, giving the path first then the contents. I also pretty consistently get this wrong, so I checked other languages and made some polls:

It seems that the other order is by far more common. Do you think it'd be worth switching over to that?

Cheers, Louis

bcpeinhardt commented 9 months ago

Yeah that's fair haha. It's set up that way with pipes in mind.

data |> write(to: filename)

But I suppose swapping them wouldn't be too disruptive to that.

data |> write(to: filename, _)

I would be open to switching the pattern around, will give it a good think.

Edit:

So It's just now occurring to me that the label provides the ordering. So with the parameters switched

let assert Ok(Nil) = "Hello" |> exp_write(to: "./tmp/test.txt")
let assert Ok("Hello") = read("./tmp/test.txt")

let assert Ok(Nil) = "./tmp/test.txt" |> exp_write(contents: "Goodbye")
let assert Ok("Goodbye") = read("./tmp/test.txt")

let assert Ok(Nil) = exp_write("./tmp/test.txt", "Bonjour")
let assert Ok("Bonjour") = read("./tmp/test.txt")

passes.

I do think the parameters being switched is the more natural ordering for the non-pipe version. I might change the function name to make it clear what's happening when piping without labels, something like

"./filename.txt" |> write_data("Hello") 

The other option to experiment with is some combinator type thing. Something like

write("Hello") |> to("./filename.txt)

or

to("./filename.txt) |> write("Hello")

that would make it impossible to misuse while still having String be the type for both parameters.

lpil commented 9 months ago

My preferences would be in this order, from favourite to least favourite:

  1. Change argument order
  2. Keep the same
  3. Builder
  4. New name

I think this because I believe a library like this is about being predictable and easy to use, and people have fairly strong opinions about what a file system API looks like. Making something unusual ought to have a strong motivator to make it worth that strangeness cost.

That new name is last as I don't think it actually solves the problem, which is evidenced by the problem already existing for the write_bits function.

write_data(filename, path)
write_data(path, filename)
bcpeinhardt commented 9 months ago

Yeah after playing with it I agree. Will change the order and do a version bump and publish this evening when I'm done with work :)

bcpeinhardt commented 9 months ago

Published :)