-
Notifications
You must be signed in to change notification settings - Fork 330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add appendFile/appendFileSync support #65
Conversation
Hi @mawni - I appreciate your support and help here. I invite you to read: #23 (comment) and #37 (comment) and I'd love to know your thoughts. |
@jprichardson thanks for considering this. I really appreciate it. So, I agree with you that Accordingly, I just tested the However, as you know better than me, For instance, in #52, as @sahas- mentioned,
and also @ajsingh007 mentioned,
and also what @dragonbanshee mentioned in #23,
The For example, const jf = require('jsonfile')
var existingFile = './somefile.json'
/*
somefile = {
garply: 'plugh',
baz: {
qux: ['corge']
}
}
*/
var moreDataToAppend = {
foo: 'bar',
baz: {
id: 09,
qux: ['waldo', 'thud']
}
}
// (array concatenation can also be provided as an option)
jf.appendFile(existingFile, moreDataToAppend, options, callback)
/*
the resulting existing file:
{
garply: 'plugh',
foo: 'bar',
baz: {
id: 09,
qux: ['corge', 'waldo', 'thud']
}
}
*/ So, please let me know what you think. Thanks. |
I think this would be really confusing, as I believe most users are thinking that appendFile would append a new JSON object to a new line. Maybe as a workaround, we just modify the README and explain both cases? Also, couldn't an const jsonFile = require('jsonfile')
jsonFile.writeFile(file, someObj, { flag: 'a' }, callback) i.e. maybe we just need to modify the docs? Thoughts? edit: This is how |
Alright, thanks for clarity. Now it makes more sense. So, @jprichardson you are right about Now, as you pointed out in the previous discussions, in all cases For example, as mentioned in ndjson,
Therefore, I confirm that it is somehow confusing and it is indeed inconsistent with In addition, about the README, I think generally it is a good idea to provide more clarity regarding this. So, what do you think then? |
I like a README update documenting the various ways to append. |
Alright, I am trying to update the README regarding
Furthermore, in those examples, what should be the extension of the resulting JSON file after appending? |
Using something like |
Where is this PR going? Is it gonna be merged soon? Seems like a useful case to me |
Closing as per discussion in #52. |
Hi @jprichardson,
this PR is regarding the issue #52 (append file support).
I've added two functions, appendFile and appendFileSync, and their tests. I've also added the node v7 to the .travis.yml and appveyor.yml.
I appreciate it if you look at them. I'd like to know what you think about it. Thanks.