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

follow_root_links() in WalkDir. #170

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

Byron
Copy link
Contributor

@Byron Byron commented Feb 2, 2023

With it it's possible to control whether symlinks in the traversal root are followed, while defaulting to 'true' like before, or if they are handled like ordinary links.

Created in response to this cargo issue.

@Byron Byron changed the title api: follow_root_links() in WalkDir. follow_root_links() in WalkDir. Feb 2, 2023
@Byron
Copy link
Contributor Author

Byron commented Feb 6, 2023

@BurntSushi I'd appreciate approval for the workflow to see if CI works. I did run it on my own fork but it seems to sometimes fail with rustup.exe now on Windows. However, I hope I will be luckier here. Thank you.

@BurntSushi
Copy link
Owner

Ug right. I really really hate the "Approve and Run" thing.

@BurntSushi
Copy link
Owner

The MSRV for walkdir is really old and I would have no problems bumping it to a newer version. But if it's just for simple/small things like [true, false] not implementing IntoIterator, then I'd prefer just leaving the MSRV alone for now.

Also, I see some lines that are over 80 columns because of long strings, and rustfmt refuses to wrap those automatically. So you might need to do it manually. :)

@Byron Byron force-pushed the follow-root-dir branch 2 times, most recently from 867c8c6 to e791c9d Compare February 6, 2023 13:34
@Byron
Copy link
Contributor Author

Byron commented Feb 6, 2023

Ug right. I really really hate the "Approve and Run" thing.

I think I addressed the issues, but… it's needing approval again 🤦. I do know that you can change this behaviour in the settings of the project though.

Screenshot 2023-02-06 at 14 36 21

Maybe this can help to make it a little less cumbersome for all of us. It feels wrong to relegate you to clicking a button, really.

@BurntSushi
Copy link
Owner

Ooooo!!!! I didn't know about that. Sucks I have to set it for every repo, but I've just made it as loose as possible.

With it it's possible to control whether symlinks in the traversal
root are followed, while defaulting to 'true' like before, or if
they are handled like ordinary links.
@Byron
Copy link
Contributor Author

Byron commented Feb 6, 2023

Great to hear I could help :)!

All tests are green now, in case you have any notes for me.

By the way, please feel free to make changes as you see fit yourself and push directly into this branch (gh pr checkout 170 sets you up for that). That way reviews tend to be way less taxing and require less (and often no) turnarounds - at least that's how I do them in my own repos.

@flokli
Copy link

flokli commented Sep 5, 2023

@BurntSushi is there anything left for this to be merged?

@BurntSushi BurntSushi merged commit dcc527d into BurntSushi:master Sep 5, 2023
@BurntSushi
Copy link
Owner

Thanks for the ping. This PR is on crates.io in walkdir 2.4.0.

@flokli
Copy link

flokli commented Sep 5, 2023

Thanks!

tvlbot pushed a commit to tvlfyi/tvix that referenced this pull request Sep 5, 2023
BurntSushi/walkdir#170 got merged, meaning we
don't need to keep our own logic in here anymore.

Our test cases already cover this.

Change-Id: Ied3043ee651c8aafa10271c1e1ca5d460fb6c0b8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9269
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants