-
Notifications
You must be signed in to change notification settings - Fork 31
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
Fix pull-platformplatform-changes CLI command on Windows #655
Fix pull-platformplatform-changes CLI command on Windows #655
Conversation
3317f54
to
37d6223
Compare
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.
Hi Tiago
Thanks for the pull request, and sorry for the late reply. I get an exception on my Mac when running this, and I had to debug to find out what was wrong. It turns out this is because the PATH environment variable on Linux and macOS is split using :
and not ;
like on Windows.
I made a comment and a suggested fix, but I will have to test that this did not break anything else. I don't have tests for the Developer CLI.
Also, I always rebase branches before merging to main, and I never merge main into a feature branch. Please rebase your changes and squash everything, including my suggested change, into a single commit, and then force push the branch to your repository.
ffc2070
to
fdd997a
Compare
Hi @tjementum I can try adding a test project to it and putting together a workflow to test this functionality initially. I can run it on Windows, Linux, and Mac. For example, I can run public static class ProcessHelperTests
{
public class StartProcessTests
{
[Theory]
[InlineData("npm -v")
public void RequestVersion_ShouldReturnValidSemver(string command)
{
// Act
var output = ProcessHelper.StartProcess(
command,
redirectOutput: true,
throwOnError: true);
// Assert
Assert.True(Version.TryParse(output, out _));
}
}
} For that, I will need to make a tiny move and change the directory structure to look like this: .
.
├─ development-cli # Contains the development CLI source code
│ ├─ cli # A .NET CLI tool for automating common developer tasks
│ ├─ tests # Tests for the CLI Do you think it makes sense? |
7f45b4a
to
cfdfdcf
Compare
69da856
to
b4aa156
Compare
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've made the requested changes and force pushed them.
…vironment variable
b4aa156
to
290139a
Compare
Summary & Motivation
While running on Windows, the
pp pull-platform platform-changes
fails because it couldn't find thenpm
command, which is not an executable file.This fix searches for the precise command location while running on Windows OS using the
.exe
and.cmd
extensions.Checklist