-
-
Notifications
You must be signed in to change notification settings - Fork 836
feat: add fs/append-file
#1967
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
feat: add fs/append-file
#1967
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 Hi there! 👋
And thank you for opening your first pull request! We will review it shortly. 🏃 💨
fs/append-file
/stdlib update-copyright-years |
Thanks, @HRIDYANSHU054, for working on this. This PR is currently failing CI. Those failures need to be addressed before the PR can be reviewed. |
Hi @kgryte , Thanks for checking this PR. I m currently working on solving these issues. Majority of them have arised because the fixtures directory was somehow not pushed to github. |
-> run_affected_benchmarks / Run affected benchmarks (pull_request) -> run_affected_examples / Run changed examples (pull_request) -> run_affected_tests / Run affected tests (pull_request)
…4/stdlib into added-fs/appendFile
@kgryte can you please have a look, I have made changes to tackle the previous PR issues. Though this might seem a silly question but why is it stuck in "Expected — Waiting for status to be reported" , is there any workaround I could do to get it cleared |
…ighlighted during the CI check
Corrected some more things. @kgryte can you once again check this |
@kgryte can you please approve the remaining workflows. I am interested in this and would like to tackle any other issue that arises or any change that is required. |
- error Begin description with `Append` instead of `Appends` stdlib/jsdoc-main-export - Expected space(s) after "catch". keyword-spacing - Unexpected sync method: 'appendFileSync'. node/no-sync
@kgryte all the issues that have been highlighted thus far during the CI checks have been solved. Can you again approve the remaining workflows so that if there are any other errors then they could also be solved and this pr can thus be reviewed. On a sidenote really thanks for your patience✨. |
- error Unnecessary try/catch wrapper no-useless-catch - error Missing JSDoc comment require-jsdoc
@kgryte can you please have a look |
@kgryte please check |
@kgryte please rerun the CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HRIDYANSHU054 I've left a number of initial comments. This implementation needs a fair amount of clean-up before it can be ready for merge.
@kgryte thanks for going through this and reviewing it. I don't why the splitting has happened in only some functions and not others even though I have configured my ESlint to highlight these issues if they occur. Also can I address this after 5th april as I am currenlty having fromm today my mid semester examinations. |
major changes - approach for sync version chnaged from throwing errors to returing errors. - approach for benchmarking, now the file is deleted after every itr and then created in the next itr by appendFile
@kgryte the changes you highlighted have been implemented, certain things like the new benchmarking approach strongly needs to be reviewed |
@kgryte can you approve these remaining workflows and then review the changes |
Hi ✋ @kgryte just hoping you could further review these changes, and anything more that you want me to change or add I will work on that also. |
hi @kgryte could you review this |
@kgryte could you review this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the late review; thanks for your PR, which looks very good now. I will merge shortly after CI has passed again after some tiny changes.
lib/node_modules/@stdlib/fs/append-file/test/fixtures/stdin_error.js.txt
Outdated
Show resolved
Hide resolved
lib/node_modules/@stdlib/fs/append-file/test/fixtures/write_error.js.txt
Outdated
Show resolved
Hide resolved
…ror.js.txt Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
…ror.js.txt Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: Philipp Burckhardt <pburckhardt@outlook.com>
Thanks @Planeshifter looking forward to learning and contribute more to stdlib node's environment |
adds file system append file utility to Stdlib
Description
This pull request:
is aligned with the purpose of achievinng feature parity with Node.js fs package. It Brings file system's powerful append file utility to Stdlib expanding its existing fs utilities.
Questions
No.
Other
Implementation details
-- async version
-- sync version
Checklist
@stdlib-js/reviewers