Allow 'firstWrite' to be passed in as 'mode'

[jonmelcher]

I have a use-case where it's useful to specify whether I am wanting to append to an already existing file on the first write.

This replaces _firstWrite with _mode, some validation to ensure that it defaults to 'w' if not passed in, otherwise it is 'w' or 'a'.

Added a couple unit tests for the new functionality, this shouldn't break the existing interface(s).

[ryu1kn]

Hi @jonmelcher , thank you for the PR!

Do you mind sharing the use-case with me? If I were to think of a drawback of this feature, the append mode could let you add lines to a different structure csv file by mistake, which cannot happen if we're using the write mode.

Is keeping the same CsvWriter instance around an option so that you can keep writing to the same file without putting an unnecessary header line? Or does the csv file already exist when your app starts up in which case you really need append mode?

[ryu1kn]

By the way, the code change itself looks good to me. I would give append: true instead of mode: "w"/"a" and only honour append param when header is not required.

[jonmelcher]

Hi @ryu1kn!

I'm gathering some metrics on a web application using lighthouse, and exporting the results in the CSV format. Most likely these CSV files will live longer than the CsvWriter and so append mode at the start would be nice. I'm currently just setting _firstWrite to the result of fs.fileExists() after construction of the CsvWriter.

I'll try to make the changes you're suggesting soon :)

[ryu1kn]

Thanks for sharing the context!

Most likely these CSV files will live longer than the CsvWriter

Yup, then we want to start with the append mode, and I feel this writing to an already existing CSV file is not an unusual use-case. I'm happy to support it.

I'll try to make the changes you're suggesting soon :)

Cool, I'm looking forward to it 😉

[jonmelcher]

@ryu1kn I've made the changes you've recommended i.e. changing mode to append and using boolean; I'm not quite sure what it means to only honour the append parameter if header is not required, but am happy to make those further changes with some explanation or pointing to some relevant codes; I couldn't find where any explicit checks for headers being required were in the code

[ryu1kn]

Thanks @jonmelcher !

I was thinking we should ignore append flag if a header is required because when you're appending CSV records to a file, you probably don't want to write a header. But when I think this again, it probably confuses users more: "why it's overwriting my file even though I'm giving append flag??"

The other options would be:

• Do not write a header if append is specified (which is what your change does)
  • User is explicitly specifying append, so ignoring header spec is not too bad.
• Raise an error if a header is required AND append is specified
  • User intention needs to be clearer, but maybe annoying if you have to change header value whether or not you want to append

To sum up, the current behaviour is better 😉

[ryu1kn]

I'm merging your change. I'll update the README, also mention to the header/append interference.

[ryu1kn]

Also updated changelog. Ready to publish a new version.
@jonmelcher, I want to know if you're ok with my reply comments and additional commits before I actually publish it 👍

[jonmelcher]

Looks great @ryu1kn! Thanks for making those changes as well :) 👍

[ryu1kn]

Cool! Just published as v1.0.0.
Thank you again for improving the library 👍