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
relative path for test dlls as arguments #335
Conversation
@@ -253,6 +253,12 @@ public void AddSource(string source) | |||
} | |||
|
|||
source = source.Trim(); | |||
|
|||
if(!FileHelper.IsRootedPath(source)) |
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.
We need tests for this
@@ -52,5 +52,17 @@ public FileAttributes GetFileAttributes(string path) | |||
{ | |||
return new FileInfo(path).Attributes; | |||
} | |||
|
|||
/// <inheritdoc/> | |||
public bool IsRootedPath(string path) |
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.
Usually we consider APIs that have filesystem interactions to be part of IFileHelper, this would allow us to inject a testable implementation easily.
Path.IsRootedPath
doesn't access the file system. It is string comparison I believe.
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.
I was considering it more of a wrapper over C# file API, so it made sense to make it a part of FileHelper
} | ||
|
||
/// <inheritdoc/> | ||
public string CombinePath(string source) |
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.
Can this API be made more intuitive? Or should we just provide an API to get current directory. Let the caller do Path.Combine
.
Path.Combine again doesn't access the file system. It may not be provided via IFileHelper
.
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.
Approach looks good to me. We should add tests in this PR.
No description provided.