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.

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(fill): Add terminal-width argument to explicitly define terminal's width #3090

Merged
merged 8 commits into from
Oct 6, 2021

Conversation

rashil2000
Copy link
Contributor

Description

This adds a new commandline option for the prompt subcommand, called --terminal-width. Without this fix, the fill module does not work on Windows as the prompt functions (of any shell) always assume a non-interactive terminal, which leads to term_size crate reporting a zero width.

This argument's value is only used as a fallback, so it will likely not affect macOS/Linux.

Motivation and Context

Closes #3086

Screenshots (if appropriate):

None

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.

@mukul-mehta
Copy link

I've tested with zsh 5.8 on macOS and it works

src/context.rs Outdated Show resolved Hide resolved
src/init/starship.bash Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@rashil2000
Copy link
Contributor Author

@davidkna Do I need to make any more 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.

Beyond my comments LGTM

src/init/starship.bash Outdated Show resolved Hide resolved
src/init/starship.fish Outdated Show resolved Hide resolved
@rashil2000
Copy link
Contributor Author

Done

@chipbuster
Copy link
Contributor

I didn't realize that rebasing onto master doesn't fix the conflict (one of the hazards of rewriting history I guess).

@rashil2000
Copy link
Contributor Author

Hey @chipbuster
I don't see any conflicts here on GitHub, could you point me towards the problem?
image

@chipbuster
Copy link
Contributor

I went ahead and resolved the merge conflicts for you (101ced5). Sorry I forgot to follow up in Discord--it was late and I was pretty tired.

@chipbuster
Copy link
Contributor

@rashil2000 Thanks for the fix!

@chipbuster chipbuster merged commit 6464693 into starship:master Oct 6, 2021
@rashil2000 rashil2000 deleted the term-width-arg branch October 7, 2021 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fill module not working on Windows
4 participants