-
-
Notifications
You must be signed in to change notification settings - Fork 13
Make DeepFS.SplitPath public & fix nested ext bug #26
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
Conversation
75e6089
to
a58e65d
Compare
@mholt what are your thoughts on the general direction this code is taking? If it looks good to you, I'll run it through some more tests, and give it another once over before removing the draft status. |
Hey, sorry, not ignoring this, just a bit behind on my notifications 😅 I have had this open in a tab for a bit now, just need to review it. Will get to it soon! :) |
No worries at all and no rush from me! I totally feel you. And aside: if you don't feel this feature in and of it's self is all that useful to your project, I can break off the bug fix into its own MR and work with what's already there in your project ^_^ DeepFS is really nifty though and I'm hoping 🤞 I can provide a few features that you like that'd make it fit my use case of just transparently traversing nested file structures in my scanner. I'm just not 100% sure I can make it fit all of the cases yet which is why I don't want to push hard for the feature and then down the road not end up using it. |
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.
Thanks for the PR!
This is my first pass of the code. It's a bit of a bigger change than I expected. Like I wasn't expecting a WalkDir method... fs.WalkDir works fine for me, so I'm not sure what that is for.
@mholt thanks for the review and questions! Responses and questions added above. I do think I like the idea of And if you want me to spend some time picking at the WalkDir situation to have it pass path lists down into the walk func instead of a custom one, just let me know ^_^ (I could also rename it to |
@mholt I'll try to knock #26 (comment) out this weekend 🤞 (Also thanks for the back and forth on this issue, I really appreciate it ^_^) |
3d046f3
to
d23304b
Compare
PathIsArchive checks to see if a path ends in an archive file. This simplifies one of the checks in the code and also fixes a small bug in DeepFS where path.Ext() wouldn't return the full extension for archive types with nested extensions: path.Ext("file.tar.gz") returns ".gz" instead of ".tar.gz"
So callers can split the path based on logic defined in the FS
d23304b
to
3e85e93
Compare
@mholt H'okay. Changes made with a much smaller diff in the project, and I think it's ready to review! I tried it out in my use case and it works well 👍 |
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.
Thanks! LGTM.
Solves #24 (a different way that is less code)
And fixes an issue where path.Ext wasn't matching against archiveExtensions because things like
path.Ext("foo.tar.gz")
returns.gz
instead of.tar.gz
.