cwalk icon indicating copy to clipboard operation
cwalk copied to clipboard

walkFunc - Should be path arg absolute?

Open saeta-eth opened this issue 6 years ago • 3 comments

Hi.

Thanks for this amazing library. It's really helpful. I was doing some tests to implement it in the project in which I work at this time.

I figured out the returned path is relative to the current directory where the scan is running. Therefore, it makes impossible to take some actions within the callback function (walkFunc).

I have changed it on my fork. Let me know if you are interested on it and I can create a PR.

saeta-eth avatar Jul 09 '19 16:07 saeta-eth

@slorenzo, right, walkFunc() currently gets a relative filepath (it is expected that you can still reconstruct the full path inside your custom walkFunc() implementation). As I re-read the documentation on the original filepath.WalkFunc(), it returns the relative path prepended with the path that was passed in the call to Walk() as the first argument. Note, however, that the returned path there is not guaranteed to be absolute; but it is expected to be resolvable related to the current working directory.

On one hand, it would make sense to adjust the behavior to fully match what filepath.Walk() does; on the other, there are use cases where having a relative file path makes sense, and I'd prefer to keep that as an option, especially for backward compatibility. So if you can make a PR that implements the filepath.Walk() path passing behavior a default but adds a boolean option, say, useRelativePaths, which, when set to true, makes the code behave the current way, such a PR would be welcomed.

iafan avatar Jul 09 '19 18:07 iafan

@iafan I'm agree with your thoughts. Would be nice to have it aligned with the original filepath.WalkFunc() because cwalk would be more reusable. Sounds good to have a useRelativePaths field to decide the behavior in order to avoid issues related to backward compatibility.

I would like to contribute to this. I will look it as soon I can.

saeta-eth avatar Jul 11 '19 04:07 saeta-eth

I would very much appreciate such a change. As cwalks API exactly resembles the API of the builtin filepath.Walk() it should also mimic its behaviour. It is very surprising when switching from the builtin to this library and then find that the returned paths are not the expected ones.

EDIT: Just to make it clear. I propose not to return absolute paths, but only the given dirpath prepended to the found filepath. This lets cwalk behave like filepath.Walk().

hupfdule avatar Nov 21 '19 14:11 hupfdule