Support shells on windows (with extensions)

[hwine]

📝 Summary

Fixes #3445

🔍 Description of Changes

Even though the value of $SHELL is just "bash" at the command line, it is the full path to the executable in the program. (i.e. 'c:\foo\bar\bash.exe'). The "name" attribute returns "bash.exe" in that case, which doesn't match "bash". Fortunately, pathlib.Path also offers a "stem" attribute which strips the extension. \o/

📋 Checklist

• [x] I have read the contributor guidelines.
• [ ] For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
• [ ] I have added tests for the changes made.
• [ ] I have run the code and verified that it works as expected.

📜 Reviewers

@akshayka OR @mscolnick

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
marimo-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 17, 2025 4:07pm
marimo-storybook	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 17, 2025 4:07pm

[vercel[bot]]

Someone is attempting to deploy a commit to the marimo Team on Vercel.

A member of the Team first needs to authorize it.

[github-actions[bot]]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[hwine]

I have read the CLA Document and I hereby sign the CLA

[mscolnick]

@hwine, are you still doing work on this (In draft), or ready to be merged?

if we wanted to update the status code to be exit(1), I am sure the test test_shell_completion would pass or we can update it to correctly test it

[hwine]

Still draft -- I'm updating the tests, will have one more commit with that. (And there's a cleaner way than exit(1) 😉 )

This PR won't change the return code behavior, as I haven't examined all the cli.py code yet.

[hwine]

Hmm. I'll have to read up a bit to avoid the macOS test failure. I know there's a way to skip for a platform, but I've not used that in parametrization before. 😞

[mscolnick]

@hwine, it looks like its returning fish, but with an unexpected path. possibly we need a different stemming util

[hwine]

@hwine, it looks like its returning fish, but with an unexpected path. possibly we need a different stemming util

I believe (and my latest commit will confirm) that the issue was expecting non-windows platforms to handle windows strings. In reality, that would never happen, so it's "safe" to skip the test on other platforms.

When I dug deeper into the logs, I found the same test failing on ubuntu as well.

[mscolnick]

Got it, sounds good. CI is running now. I'll merge once it passes

Thanks so much!

[hwine]

PEBKAC: I found the correct run and it passes (or is skipped appropriately) on all tested platforms.

original incorrect analysis

@mscolnick huh - I'm confused -- I don't see any of my tests failing on macos, but I can't find successes, either!

I would expect to find it reported just below this line. But I'm also not clear on your tests (many omit test_cli*).

Any suggestions on forcing the tests to run? Or are they reported somewhere else?

[mscolnick]

@hwine , yep, all looks good. just some flakey tests (which i am fixing now)

[github-actions[bot]]

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.10.14-dev35