Skip to content
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

path/filepath: add SkipAll #47209

Closed
mvdan opened this issue Jul 14, 2021 · 17 comments
Closed

path/filepath: add SkipAll #47209

mvdan opened this issue Jul 14, 2021 · 17 comments

Comments

@mvdan
Copy link
Member

mvdan commented Jul 14, 2021

This is a re-opening of #43760, since the original author closed it - seemingly for their perception that there was no interest. The proposed API is small, the issue had six upvote reactions at the time of closing, and I personally think it's a good idea too, so I'm reopening here.

Since it's possible to delete GitHub comments or entire threads, I've made a waybackmachine copy as of today: https://web.archive.org/web/20210714212659/https://github.com/golang/go/issues/43760

In summary: Right now we have filepath.SkipDir for skipping an entire directory while walking via filepath.Walk, but nothing for stopping the walk entirely. It's possible to accomplish that by returning a custom error and ignoring it from the filepath.Walk error return, but that's a bit clunky. It would be nice to simply use StopWalk, and have Walk return a nil error when it's stopped that way.

The original issue was a proposal, but I don't think this is complex or controversial enough to need the process. I also think waiting to be selected for proposal review might have been what caused the confusion around the lack of interest.

cc @89z @ianlancetaylor @josharian
cc @robpike @rsc as per https://dev.golang.org/owners

@mvdan mvdan added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. FeatureRequest labels Jul 14, 2021
@mvdan

This comment has been minimized.

@mvdan
Copy link
Member Author

mvdan commented Jul 14, 2021

Upon closer inspection, it does look like some replies in the other thread from the original author were deleted, so it almost looks like Ian and me were talking to ourselves. They don't affect the proposed API at all, though, since they were just talking about why the issue hadn't gotten a resolution yet.

@cherrymui cherrymui changed the title path/filepath: add StopWalk proposal: path/filepath: add StopWalk Jul 14, 2021
@gopherbot gopherbot added this to the Proposal milestone Jul 14, 2021
@gopherbot gopherbot added Proposal and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jul 14, 2021
@mvdan
Copy link
Member Author

mvdan commented Jul 14, 2021

@cherrymui any particular reason you turned this into a proposal? I particularly wanted to avoid the red tape of waiting for the proposal review committee to pick this up :)

@cherrymui
Copy link
Member

The original issue is a proposal and I'm not sure why this is not. I though all new APIs are proposals?

If you don't think this is a proposal, feel free to modify. Thanks.

@mvdan
Copy link
Member Author

mvdan commented Jul 14, 2021

See this bit from the top of this thread:

The original issue was a proposal, but I don't think this is complex or controversial enough to need the process. I also think waiting to be selected for proposal review might have been what caused the confusion around the lack of interest.

I seem to recall Russ has said that the proposal review committee only needs to be looped in if a feature/API request isn't getting attention from the owners of the package. I guess there's no harm in making it a proposal, since both owners are likely busy.

@rsc
Copy link
Contributor

rsc commented Jul 19, 2021

All new public API should go through the proposal process. Feel free to ping us if something urgent is not being picked up. Thanks.

@rsc rsc moved this from Incoming to Active in Proposals (old) Sep 1, 2021
@rsc
Copy link
Contributor

rsc commented Sep 1, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Sep 8, 2021

Should it be StopWalk or use the same Skip prefix that SkipDir does, like SkipAll?

@rsc
Copy link
Contributor

rsc commented Sep 8, 2021

/cc @robpike

@robpike
Copy link
Contributor

robpike commented Sep 8, 2021

SkipAll is more consistent, StopWalk is more clear. Either is fine.

@frioux
Copy link

frioux commented Sep 9, 2021

I'd vote StopWalk

@deefdragon
Copy link

I feel StopWalk is the better choice.

SkipAll implies to me that it still scans/reads everything, but just doesn't do the callback, whereas StopWalk implies a full stop.

@rogpeppe
Copy link
Contributor

rogpeppe commented Sep 12, 2021

On balance my vote is for SkipAll. Just as we can assume that SkipDir avoids scanning of the directory contents, it seems reasonable to assume that SkipAll wouldn't do any more scanning at all.

Also:

  • it means it will always sit next to the other error value in the docs
  • the similarity in name implies a kinship of functionality (both are sentinel values interpreted by Walk)

@rsc
Copy link
Contributor

rsc commented Sep 15, 2021

SkipAll it is.

@rsc rsc changed the title proposal: path/filepath: add StopWalk proposal: path/filepath: add SkipAll Sep 15, 2021
@rsc
Copy link
Contributor

rsc commented Sep 15, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Sep 15, 2021
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Sep 22, 2021
@rsc
Copy link
Contributor

rsc commented Sep 22, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: path/filepath: add SkipAll path/filepath: add SkipAll Sep 22, 2021
@rsc rsc modified the milestones: Proposal, Backlog Sep 22, 2021
@gopherbot
Copy link

Change https://golang.org/cl/363814 mentions this issue: path/filepath, io/fs: add SkipAll

rajbarik pushed a commit to rajbarik/go that referenced this issue Sep 1, 2022
Fixes golang#47209

Change-Id: If75b0dd38f2c30a23517205d80c7a6683a5c921c
Reviewed-on: https://go-review.googlesource.com/c/go/+/363814
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

8 participants