Skip to content

update fs doc for filter - #49289

Merged
aduh95 merged 2 commits into
nodejs:mainfrom
shubham9411:updating-fs-filter-doc
Aug 26, 2023
Merged

update fs doc for filter#49289
aduh95 merged 2 commits into
nodejs:mainfrom
shubham9411:updating-fs-filter-doc

Conversation

@shubham9411

@shubham9411shubham9411 commented Aug 22, 2023•
edited by aduh95
Loading

Copy link
Copy Markdown
Contributor

Updating doc for fs.cp, fsPromises.cp and fs.cpSync.

Added the caveat that filter will be applied to both files and dirs.

Fixes: #49092

@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Aug 22, 2023
@shubham9411shubham9411 mentioned this pull request Aug 22, 2023
Closed
tniessen
tniessen reviewed Aug 22, 2023
View reviewed changes

@tniessentniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems extremely long and verbose for a rather simple statement. The documentation already says that the function will be applied to files and directories.

Comment threaddoc/api/fs.md Outdated
@aduh95aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 23, 2023
tniessen
tniessen approved these changes Aug 23, 2023
View reviewed changes

@tniessentniessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: commit message does not adhere to the commit message guidelines

@shubham9411
shubham9411force-pushed the updating-fs-filter-doc branch from 7a4107b to 7898fe0CompareAugust 23, 2023 13:31
@LiviaMedeiros

LiviaMedeiros commented Aug 24, 2023

Copy link
Copy Markdown
Member

The commit message guidelines suggest that it should start with an imperative verb, so maybe something like

doc: update doc for fs.cp, fsPromises.cp and fs.cpSync

or

doc: update options.filter description for fs.cp

Would be slightly better.

@shubham9411

shubham9411 commented Aug 24, 2023

Copy link
Copy Markdown
ContributorAuthor

@LiviaMedeiros yes, I had updated first commit to
doc: update doc for fs.cp, fsPromises.cp and fs.cpSync
😄

@aduh95
aduh95 merged commit 4178683 into nodejs:mainAug 26, 2023
@aduh95

aduh95 commented Aug 26, 2023

Copy link
Copy Markdown
Contributor

Landed in 4178683, thanks for the contribution :)

UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@shubham9411@UlisesGascon
doc: update options.filter description for fs.cp
7c0cd2f
PR-URL: #49289Fixes: #49092
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
@UlisesGasconUlisesGascon mentioned this pull request Sep 10, 2023
Merged
targos pushed a commit that referenced this pull request Nov 27, 2023
@shubham9411@targos
doc: update options.filter description for fs.cp
0dda724
PR-URL: #49289Fixes: #49092
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
@targostargos mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
doc: update options.filter description for fs.cp
7e5adf2
PR-URL: nodejs/node#49289Fixes: nodejs/node#49092
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
doc: update options.filter description for fs.cp
4ea512a
PR-URL: nodejs/node#49289Fixes: nodejs/node#49092
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
@coderabbitaicoderabbitaiBot mentioned this pull request Jul 29, 2026
Merged
5 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lpincalpincalpinca approved these changes
@tniessentniessentniessen approved these changes
@aduh95aduh95aduh95 approved these changes
@LiviaMedeirosLiviaMedeirosLiviaMedeiros approved these changes
+1 more reviewer
@aymen94aymen94aymen94 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.docIssues and PRs related to the documentations.fsIssues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

"Experimental" fs.cp(), fs.cpSync(), fsPromises.cp() methods don't handle recursion when there is a filter

7 participants

@shubham9411@LiviaMedeiros@aduh95@lpinca@tniessen@aymen94@nodejs-github-bot