Documented Path.Join #563
Documented Path.Join #563
Conversation
rpetrusha
added
the
🚧 Hold for related PR
| <summary>To be added.</summary> | ||
| <returns>To be added.</returns> | ||
| <remarks>To be added.</remarks> | ||
| <param name="path">A relative path to concatenate to <see paramref="basePath" />.</param> |
There was a problem hiding this comment.
Can we add some ## Examples for this overload as well? I think they really help illustrate how to use this method.
There was a problem hiding this comment.
The key thing this is supposed to replace is depending on the current working directory.
Environment.CurrentDirectory = @"C:\temp";
// Current directory can change at any time by any thread in the process so this isn't deterministic.
string path = Path.GetFullPath(relativePath);
// Deterministic version (thread safe)
path = Path.GetFullPath(@"C:\temp", relativePath);If you have any "relative" paths the new overload is the safe alternative. We also have a method for determining if the current directory will change paths when resolved/used: Path.IsPathFullyQualified
// If this returns true the current directory will not change the location the path points to
bool currentDirectorySafe = Path.IsPathFullyQualified(path);MSBuild reading project files would be one concrete example of where this API should be used for thread safety. Any given path in a project file should be resolved (evaluated in their terms) using Path.GetFullPath(projectFilePath, itemPath).
There was a problem hiding this comment.
A thing with this API that I really struggle with is that even you switched the parameters around in your write-up above....
path = Path.GetFullPath(@"C:\temp", relativePath);
But the signature is:
Path.GetFullPath (string path, string basePath)
|
I can't still review this one because the samples PR hasn't been merged yet @rpetrusha. |
|
@JeremyKuhne, @mairaw, and @BillWagner, this will be ready for review once dotnet/samples#328 merges. |
|
Closing and reopening to begin new build after merge of dotnet/samples#328. |
BillWagner
left a comment
BillWagner
left a comment
There was a problem hiding this comment.
These are great updates @rpetrusha
I had a few small suggestions, but this is ready to go. Once a build finishes, I'll approve.
xml/System.IO/Path.xml
Outdated
| [!code-csharp[System.IO.Path Members#13](~/samples/snippets/csharp/VS_Snippets_CLR_System/system.IO.Path Members/CS/pathmembers.cs#13)] | ||
| [!code-vb[System.IO.Path Members#13](~/samples/snippets/visualbasic/VS_Snippets_CLR_System/system.IO.Path Members/VB/pathmembers.vb#13)] | ||
| The following example displays <xref:System.IO.Path> field values on Windows and on Linux systems. Note that Windows supports either the forward slash (which is returned by the <xref:System.IO.Path.AltDirectorySeparatorChar> field) or the backslash (which is returned by the <xref:System.IO.Path.DirectorySeparatorChar> field) as path separator characters, while Linux supports only the forward slash. |
There was a problem hiding this comment.
I'd recommend using "unix" instead of "Linux". That would cover MacOS as well.
xml/System.IO/Path.xml
Outdated
| This field can have the same value as <xref:System.IO.Path.DirectorySeparatorChar>. `AltDirectorySeparatorChar` and <xref:System.IO.Path.DirectorySeparatorChar> are both valid for separating directory levels in a path string. | ||
| The value of this field is a slash ('/') on the Windows, UNIX and Macintosh operating systems. |
There was a problem hiding this comment.
Macintosh is a UNIX system, so you can remove the specific callout to Mac.
| - If you prefer to hard-code the directory separator character, you should use the forward slash (`/`) character. It is the only recognized directory separator character on Unix systems, as the output from the example shows, and is the <xref:System.IO.Path.AltDirectorySeparatorChar> on Windows. | ||
| - Use string concatenation to dynamically retrieve the path separator character at runtime and incorporate it into file system paths. For example, |
There was a problem hiding this comment.
Suggestion: When I might persist path information, I use the AltSeparatorChar because it will be the / on both platforms. I only do that when the path info must be cross plat.
BillWagner
left a comment
BillWagner
left a comment
There was a problem hiding this comment.
This LGTM @rpetrusha
I ha a few small comments, but you can 
when you are ready.
mairaw
removed
the
🚧 Hold for related PR
5 participants
Documented Path.Join
The PR:
Related to dotnet/samples#328
Fixes #970
Fixes #1213
Fixes #877
//cc @JeremyKuhne