cristianbuse / VBA-FileTools

Useful methods for interacting with the file system
MIT License
81 stars 23 forks source link

Update LibFileTools.bas Added PathJoin Function #14

Closed Kaydron1000 closed 1 year ago

Kaydron1000 commented 1 year ago

Added PathJoin function. Allows multiple pieces of a path to be joined together.

I find this style of function to be more useful than the BuildPath function. I will leave up to you on how (or if) to include it in your source.

cristianbuse commented 1 year ago

Hi @Kaydron1000 ,

Thank you for your contribution!

I am not sure you looked into what FixPathSeparators does but it removes the need to strip leading and trailing slashes in all of the components. This means that we can rewrite your function to just this:

Public Function PathJoin(ParamArray PathComponents() As Variant) As String
    PathJoin = FixPathSeparators(Join(PathComponents, PATH_SEPARATOR))
End Function

Now the real question is if this wrapping is really needed. Consider the next 2 lines which do the exact same thing:

Sub Test()
    Debug.Print FixPathSeparators(Join(Array("Test\", "\\Test", "\/Test"), PATH_SEPARATOR))
    Debug.Print PathJoin("Test\", "\\Test", "\/Test")
End Sub

In a way, there is no need for the PathJoin function as that can be achieved with FixPathSeparators(Join(Array(.... However, I really like the abstraction and I wonder why I haven't thought of it myself :smile:.

I would suggest renaming to JoinPath to keep the naming convention consistent with the rest of the methods i.e. verb first. What do you think?

Kaydron1000 commented 1 year ago

I did see the FixPathSeparators function but I did not dig too much into it. I was originally throwing this out there to get an initial opinion. Your BuildPath function does something similar but only accepts 2 parameters. I was thinking this idea would replace or add to it. I usually do not use Array(), but your solution used it well and fits rather nicely into what is already available.

As for PathJoin vs JoinPath, I agree JoinPath to match the conventions of this file. C# has Path.Join() which contains this functionality hence I initially named it PathJoin.

Additional note there is a built in Path Separator in the application. I assume it acts correctly for mac vs. PC, but not sure if it is available in all applications. (Application.PathSeparator)

I committed changes to the pull request. I went with removing PathJoin/JoinPath and updating your BuildPath function to accept ParamArray.

Public Function BuildPath (ParamArray PathComponents() As Variant) As String
    BuildPath = FixPathSeparators(Join(PathComponents, PATH_SEPARATOR))
End Function
cristianbuse commented 1 year ago

Indeed the Application.PathSeparator is not available in all apps. I actually had a wrapper around it and replaced it in 0552a11e3a29f144f08e7446f931748cc0794536. The main reasoning is that the constant already works for both Win and Mac and I wanted to avoid the extra stack frame for every time the separator is used. The only times the constant would not work would be for some Japanese and Korean system locals as in here.

I was actually thinking of updating BuildPath as well rather than introduce a new method. Would also be backward compatible if named parameters were not used.

Many thanks!

guwidoe commented 1 year ago

Hi Cristian, I think you are mistaken that the constant approach won't work for Japanese and Korean system locales, even though they seem to use ¥ and as path separators.

If you change your system locale to Japanese you will instead see your constant magically turning into a ¥ because the codepoint 92 (\ in ASCII) maps to ¥ in CP932, the codepage used for non-Unicode apps on Windows with Japanese system locale. This is actually the reason why these funky-looking path separators came to be in the first place (¥ for Japan and for Korea).

cristianbuse commented 1 year ago

@guwidoe

Thanks Guido,

Good to know. I haven't bothered checking 😄