Add trailing slash for -o Fixes #45682#48640
Conversation
ghost
added
There was a problem hiding this comment.
Pull Request Overview
This PR fixes #45682 by updating the -o option parser to ensure that supplied paths have a trailing directory separator.
- Adds a custom parser to the output option
- Appends a trailing slash if it is missing from the provided path
Comments suppressed due to low confidence (1)
src/Cli/Microsoft.TemplateEngine.Cli/Commands/SharedOptions.cs:18
- [nitpick] Consider renaming 'input' to 'inputPath' for improved clarity on the expected data.
var input = argumentResult.Tokens[0]?.Value;
| return new FileInfo(input); | ||
| } | ||
| else | ||
| { | ||
| return new FileInfo(input + Path.DirectorySeparatorChar); |
There was a problem hiding this comment.
Appending a directory separator and returning a FileInfo could be problematic if the intent is to work with directories; consider using DirectoryInfo instead.
| return new FileInfo(input); | |
| } | |
| else | |
| { | |
| return new FileInfo(input + Path.DirectorySeparatorChar); | |
| return new DirectoryInfo(input); | |
| } | |
| else | |
| { | |
| return new DirectoryInfo(input + Path.DirectorySeparatorChar); |
| return null; | ||
| } | ||
| else if (Path.EndsInDirectorySeparator(input)) | ||
| { |
There was a problem hiding this comment.
Could simplify the else with something like
return new FileInfo(input.TrimEnd(Path.DirectorySeparateChar)+Path.DirectorySeparateChar);
| Arity = new ArgumentArity(1, 1), | ||
| CustomParser = (ArgumentResult argumentResult) => | ||
| { | ||
| var input = argumentResult.Tokens[0]?.Value; |
There was a problem hiding this comment.
if the argument is null/empty, shouldn't the parser raise an error or is that checked before hitting the custom parser?
|
I tried this out in a more meaningful way, and it didn't work as I'd expected. It turns out CustomParsers don't really do anything as far as the output of the parser (??!!) because after it gets the result here, it just throws it away. I'm restarting this PR but with a different change in a different place that should affect commands not connected with the template engine. |
This reverts commit 105a59f.
Did you disable it universally or just for full framework legs? If it's not a huge pain keeping it on for legs where it's working is a nice backstop. |
I can try to make it framework-specific. It's currently all frameworks. |
|
Note: PR is ready for review of the final changes. sounds like baronfel agreed with this approach to that test. |
|
Due to lack of recent activity, this PR has been labeled as 'Stale'. It will be closed if no further activity occurs within 7 more days. Any new comment will remove the label. |
|
This PR is perfectly good and should be ready to merge. Sorry I missed it being marked stale. |
# Conflicts: # test/dotnet.Tests/CommandTests/MSBuild/GivenDotnetBuildInvocation.cs # test/dotnet.Tests/CommandTests/MSBuild/GivenDotnetStoreInvocation.cs
|
@Forgind I got the PR conflicts fixed up and merged for ya. |
|
Thanks! Hope there weren't too many merge conflicts 🙂 |
Fixes #45682 by tweaking the value of the argument after it's parsed.