Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(shell): allow distinguishing between pwsh and powershell #5478

Merged
merged 5 commits into from Nov 25, 2023
Merged

feat(shell): allow distinguishing between pwsh and powershell #5478

merged 5 commits into from Nov 25, 2023

Conversation

HeyItsGilbert
Copy link
Contributor

@HeyItsGilbert HeyItsGilbert commented Oct 3, 2023 •
edited by davidkna

Description

pwsh.exe and powershell.exe are currently both identified as powershell. This PR intends to distinguish between the two in the shell module (as well as make the necessary context changes to allow for that)

This is a copy of #5031 which was in draft. Hopefully I did this right....
This is a redo of #5156.

Motivation and Context

Distinguishes between pwsh and powershell shell instances. A Windows machine can have both and having a clear distinction can avoid issues.

Closes #5031

Screenshots (if appropriate):

How Has This Been Tested?

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@HeyItsGilbert HeyItsGilbert mentioned this pull request Oct 3, 2023
Closed
5 tasks
@HeyItsGilbert HeyItsGilbert force-pushed the distinguish-between-pwsh-and-powershell branch from 051455d to e8792a8 Compare October 3, 2023 04:35
davidkna
davidkna requested changes Oct 3, 2023
View reviewed changes
Copy link
Member

@davidkna davidkna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid unexpected changes to user-configured shell-module configs, please default powershell_indicator to the value of powershellcore_indicator. I have provided some suggestions to that end, but please also implement tests for the behavior.

Please also update the English shell module documentation.

src/configs/shell.rs Outdated Show resolved Hide resolved
src/configs/shell.rs Outdated Show resolved Hide resolved
src/modules/shell.rs Outdated Show resolved Hide resolved
src/modules/shell.rs Outdated Show resolved Hide resolved
src/formatter/string_formatter.rs Outdated Show resolved Hide resolved
@HeyItsGilbert HeyItsGilbert marked this pull request as draft October 3, 2023 15:07
@HeyItsGilbert
Copy link
Contributor Author

To avoid unexpected changes to user-configured shell-module configs, please default powershell_indicator to the value of powershellcore_indicator.

I'm assuming you mean default powershellcore to powershell. PowerShell is what most folks call Windows PowerShell. Where as the open source .Net core version (that's cross platform) is called PowerShell Core. In my previous PR I actually renamed powershell_indicator to Windows PowerShell, but I think there was some concern with that. I'll bring this back up in the issue, to make sure we're all on the same page before I put this back up for review, etc.

@HeyItsGilbert HeyItsGilbert mentioned this pull request Oct 3, 2023
Closed
@davidkna
Copy link
Member

davidkna commented Oct 3, 2023

I'm assuming you mean default powershellcore to powershell.

Yes.

@HeyItsGilbert HeyItsGilbert marked this pull request as ready for review October 4, 2023 18:46
@HeyItsGilbert
Distinguish between pwsh and powershell
b0a4027 
Signed-off-by: Gilbert Sanchez <me@gilbertsanchez.com>
- Update the config-schema
- Apply suggestions from code review
- Revert string formatter tests
- Rename to use pwsh
- Fix config-schema after rename.
- Fix tests after rename
@HeyItsGilbert HeyItsGilbert force-pushed the distinguish-between-pwsh-and-powershell branch from 843a266 to b0a4027 Compare October 13, 2023 15:19
@HeyItsGilbert
Copy link
Contributor Author

Rebased to hopefully resolve the nightly failed tests which are unrelated to my change but ¯_(ツ)_/¯. This is ready @davidkna.

davidkna
davidkna reviewed Oct 15, 2023
View reviewed changes
docs/config/README.md Outdated Show resolved Hide resolved
src/modules/shell.rs Show resolved Hide resolved
davidkna
davidkna reviewed Oct 30, 2023
View reviewed changes
Copy link
Member

@davidkna davidkna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some suggestions to the docs, but otherwise this looks good.

docs/config/README.md Outdated Show resolved Hide resolved
docs/config/README.md Outdated Show resolved Hide resolved
HeyItsGilbert and others added 2 commits October 31, 2023 14:11
@HeyItsGilbert @davidkna
Update docs/config/README.md
858744e 
Co-authored-by: David Knaack <davidkna@users.noreply.github.com>
@HeyItsGilbert @davidkna
Update docs/config/README.md
feedd19 
Co-authored-by: David Knaack <davidkna@users.noreply.github.com>
@davidkna
Copy link
Member

davidkna commented Nov 2, 2023

LGTM if you run dprint fmt on the config file.

@HeyItsGilbert
Copy link
Contributor Author

Checking in @davidkna. Let me know if I need anything else.

@davidkna davidkna changed the title Distinguish between pwsh and powershell feat(shell): allow distinguishing between pwsh and powershell Nov 20, 2023
@davidkna davidkna merged commit d7a34b4 into starship:master Nov 25, 2023
17 of 20 checks passed
@davidkna
Copy link
Member

Thanks for adding this feature @HeyItsGilbert!

@github-actions github-actions bot mentioned this pull request Nov 25, 2023
Merged
@HeyItsGilbert HeyItsGilbert deleted the distinguish-between-pwsh-and-powershell branch November 25, 2023 16:26
@chiefmikey chiefmikey mentioned this pull request Dec 28, 2023
Merged
@NikGovorov
Copy link

@HeyItsGilbert thanks for your contribution. However, your change affects another awesome recently added feature #5049. Could you please add Shell:Pwsh to:

starship/src/modules/character.rs

Line 40 in fd32e35

| (Shell::Cmd | Shell::PowerShell, "vi") => ShellEditMode::Normal,

HeyItsGilbert added a commit to HeyItsGilbert/starship that referenced this pull request Feb 14, 2024
@HeyItsGilbert
Add missing vi for char for Shell::Pwsh
847863a 
starship#5478 (comment)
@HeyItsGilbert HeyItsGilbert mentioned this pull request Feb 14, 2024
Merged
5 tasks
davidkna pushed a commit that referenced this pull request Feb 19, 2024
@HeyItsGilbert
fix(character): also handle vi edit mode in pwsh (#5775)
0891ec2 
* Add missing vi for char for Shell::Pwsh

#5478 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidkna davidkna davidkna approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@HeyItsGilbert @davidkna @NikGovorov