Implement windows-powershell setting

[michidk]

Implement the windows-powershell setting to fix #1050.

To be able to run the same justfile on Linux and Windows set shell is not enough because it would change the shell on every platform. To set PowerShell as the default shell when running just on Windows set windows-powershell to true.

set windows-powershell
hello-windows:
  Write-Host "Hello, world!"
hello-linux:
  echo 'Hello, world!'

[michidk]

Hey @casey I finished the implementation of windows-powershell. The core logic is here: https://github.com/michidk/just/blob/windows-powershell/src/settings.rs#L41
But quite a few integration tests like byte_order_mark::ignore_leading_byte_order_mark started failing after I implemented the shell selection logic (see GitHub actions).
I am not entirely sure what those tests do and what my code has do to with them failing, but I noticed in the test results that bash is used sometimes. Not sure if that's intended or some fallback in Linux?
If you have some time over the course of the next weeks, maybe you can have a look at it.

Happy new year!

[casey]

It's not a very satisfying answer, but I think if a whole bunch of tests start failing, the best thing to do is to uncomment/revert code until you they all pass/you get the previous behavior, and then start modifying it incrementally until you get failing tests.

[casey]

#1061 reminded me of something: If there are arguments that make powershell less error prone, we should pass them by default. So, for example, if there's something like -u, which makes evaluating undefined variables an error, we should pass that.

[michidk]

#1061 reminded me of something: If there are arguments that make powershell less error prone, we should pass them by default. So, for example, if there's something like -u, which makes evaluating undefined variables an error, we should pass that.

I don't think there is such an argument. I think I will add the -NoLogo argument though. The argument list can be found here: https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_powershell_exe?view=powershell-5.1

[michidk]

@casey I changed the version just-variable into an environment variable in the main justfile of this project itself because it would try to execute sed which is not available on Windows. Now I am able to execute most commands like just fmt.
Not sure if you agree with that change though, so please have a look at it and let me know what you think: michidk@1dc2502

[casey]

I don't think the main justfile should be changed in this PR. In general, I try to keep PRs as small as possible, so they can be reviewed easily and merged quickly. Changing the main justfile to use Powershell would be a large change, since all the recipes would need to be rewritten to be compatible with Powershell, so it should be left out of this PR.

[michidk]

I don't think the main justfile should be changed in this PR. In general, I try to keep PRs as small as possible, so they can be reviewed easily and merged quickly. Changing the main justfile to use Powershell would be a large change, since all the recipes would need to be rewritten to be compatible with Powershell, so it should be left out of this PR.

I actually used this justfile for quite some time now. It works great - I never needed the parts that didn't work with PowerShell. But then I will keep it locally because I don't have the time to translate the whole justfile. Just as a sidenote: If variables would evaluate lazy (as soon as they are needed, and not before that) then the file in the current master would also work for most commands like just fmt.

[michidk]

I reverted the previous commit, so that the justfile now matches with the one from the current master again (meaning it's unchanged in this PR).

I would consider the PR as ready for review now. Thanks to @Shemnei who worked with me on this feature.

[casey]

Nice, this mostly looks good. Thanks for writing all the unit tests!

As for integration tests, there should be a new integration test dedicated to this feature, which I suggested in a comment, and none of the existing integration tests should change.

The reason being that when reviewing a PR like this, you really want to be sure that the feature works, and that nothing else changed. The easiest way to do this is to leave the existing tests alone, and write a new test that just covers the new feature.

[michidk]

@casey Thanks for your review! I implemented all the changes.

[casey]

Getting there!

[casey]

I think this conversation was accidentally marked as resolved. Since it's an array of strings, I would call it DEFAULT_SHELL_ARGS even if there's only one.

[michidk]

Done!

[casey]

Nice, merged! Thanks for sticking with it! This is a great feature, and long overdue.

[michidk]

Thanks for reviewing!