Closed jonmelcher closed 6 years ago
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?
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.
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 :)
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 😉
@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
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:
append
is specified (which is what your change does)
append
, so ignoring header spec is not too bad.append
is specified
header
value whether or not you want to appendTo sum up, the current behaviour is better 😉
I'm merging your change. I'll update the README, also mention to the header
/append
interference.
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 👍
Looks great @ryu1kn! Thanks for making those changes as well :) :+1:
Cool! Just published as v1.0.0. Thank you again for improving the library 👍
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).