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

fix(python): Add a missing ' ' to the end of the python prompt #2248

Merged
merged 1 commit into from
Mar 24, 2021
Merged

fix(python): Add a missing ' ' to the end of the python prompt #2248

merged 1 commit into from
Mar 24, 2021

Conversation

offbyone
Copy link
Contributor

@offbyone offbyone commented Jan 30, 2021 •
edited by andytom
Loading

Description

After upgrading from 0.47 to 0.49 I noticed that there was a missing ' ' between
the python format section and the one that comes after that.

Motivation and Context

Fixes #2342

Screenshots (if appropriate):

# this is with 0.49 release
~/p/ideas on  master [$?] via ⬢ v15.3.0 via 🐍 v3.9.1 (python-3.9.1)via 💠 default on ☁️  me(pdx) on ☁️   8 🐟
❯ ../starship/target/debug/starship prompt

# this is with this patch
~/p/ideas on  master [$?] via ⬢ v15.3.0 via 🐍 v3.9.1 (python-3.9.1) via 💠 default on ☁️  me(pdx) on ☁️   8 🐟
❯ ⏎

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.

andytom
andytom reviewed Jan 30, 2021
View reviewed changes
Copy link
Member

@andytom andytom left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for fixing this @offbyone, I have left a suggestion, can you also update the tests and the documentation (you only need to update the English version the rest will be done automatically).

src/configs/python.rs Outdated
@@ -21,7 +21,7 @@ impl<'a> RootModuleConfig<'a> for PythonConfig<'a> {
pyenv_prefix: "pyenv ",
python_binary: VecOr(vec!["python", "python3", "python2"]),
scan_for_pyfiles: true,
format: "via [${symbol}${pyenv_prefix}(${version} )(\\($virtualenv\\))]($style)",
format: "via [${symbol}${pyenv_prefix}(${version} )(\\($virtualenv\\))]($style) ",
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be

Suggested change
format: "via [${symbol}${pyenv_prefix}(${version} )(\\($virtualenv\\))]($style) ",
format: "via [${symbol}${pyenv_prefix}(${version} )(\\($virtualenv\\) )]($style)",

Otherwise there would be too many spaces if the virtualenv is not activated but there is a python file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do!

@offbyone offbyone force-pushed the python-format-fix branch 2 times, most recently from 0c0393e to da8fdcb Compare January 30, 2021 21:02
@offbyone
Copy link
Contributor Author

The test that fails passed in the last run, which differs only in its commit message. I'm pretty sure that this was a nondeterministic test run.

@chipbuster
Copy link
Contributor

The test that fails passed in the last run, which differs only in its commit message. I'm pretty sure that this was a nondeterministic test run.

Yeah, we've been fighting issues with that on-and-off for the last year. The only environments the race conditions have appeared in are GitHub Action and an s390x, which has on the order of 300 threads of actual parallelism.

I've restarted a test run for you.

@andytom andytom changed the title Add a missing ' ' to the end of the python prompt Fix(python): Add a missing ' ' to the end of the python prompt Jan 30, 2021
@andytom andytom changed the title Fix(python): Add a missing ' ' to the end of the python prompt fix(python): Add a missing ' ' to the end of the python prompt Jan 30, 2021
@andytom
Copy link
Member

andytom commented Feb 21, 2021

Hi @offbyone, there have been some changes to the python module that means that there are some merge conflicts would you be able to fix them? Also can you update the default format string in the docs?

@ericbn
Copy link
Contributor

ericbn commented Mar 6, 2021

@offbyone, do you plan to update your PR? I'm more than happy to help rebasing the changes and also updating the docs accordingly.

@offbyone
Copy link
Contributor Author

offbyone commented Mar 8, 2021

Yeah, I plan to rebase it, sorry -- Real Life™ has interfered.

@offbyone offbyone force-pushed the python-format-fix branch from da8fdcb to b590303 Compare March 8, 2021 09:15
@offbyone
Copy link
Contributor Author

@ericbn I have rebased it; is this mergeable now?

@andytom
Copy link
Member

andytom commented Mar 13, 2021 •
edited
Loading

@ericbn I have rebased it; is this mergeable now?

The only thing that is missing that I can see is to update the default in the English version of the docs. Other than that we should be good to go.

@offbyone offbyone force-pushed the python-format-fix branch from b590303 to ff47295 Compare March 24, 2021 07:23
@offbyone offbyone force-pushed the python-format-fix branch from ff47295 to c8161e8 Compare March 24, 2021 07:27
@offbyone
Copy link
Contributor Author

There we go: docs updated too.

vladimyr
vladimyr approved these changes Mar 24, 2021
View reviewed changes
Copy link
Member

@vladimyr vladimyr left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@offbyone
Copy link
Contributor Author

Would someone with permissions care to merge this?

@chipbuster chipbuster merged commit aa7a805 into starship:master Mar 24, 2021
@offbyone offbyone deleted the python-format-fix branch March 24, 2021 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andytom andytom andytom left review comments

@vladimyr vladimyr vladimyr 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.

formatting issue with space for python
5 participants
@offbyone @chipbuster @andytom @ericbn @vladimyr