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

feat: Add xonsh support #1265

Closed
wants to merge 6 commits into from
Closed

Conversation

vladimyr
Copy link
Member

Description

Adds xonsh init script.

Motivation and Context

Closes #272

Screenshots (if appropriate):

image

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 requested a review from a team May 28, 2020 07:06
@andytom andytom changed the title Add xonsh support feat: Add xonsh support May 28, 2020
Copy link
Member

@matchai matchai left a comment

Choose a reason for hiding this comment

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

The PR looks great to me! Thank you for taking the time to add docs everywhere they're needed. 😄

Unfortunately, I seem to be having issues getting starship working, following the provided instructions.
I appear to get the following error after editing ~/.xonshrc or running execx($(starship init xonsh)) directly:

NameError: name 'get_next_job_number' is not defined

I see the get_next_job_number function is clearly imported at the start of your init file, but I'm not familiar enough with xonsh to know where to start troubleshooting.

I have installed xonsh directly from Brew (xonsh/0.9.18) if that's of any help.
If I can provide any details to help troubleshoot, let me know. If you'd prefer more synchronous communication while troubleshooting, I'm always available on the project Discord: https://discord.gg/8Jzqu3T

@vladimyr
Copy link
Member Author

The PR looks great to me! Thank you for taking the time to add docs everywhere they're needed. 😄

I'm giving my best 😁

I have installed xonsh directly from Brew (xonsh/0.9.18) if that's of any help.

Well, we took different routes. I've installed it using pipx. I'll try with Homebrew and let you know.

If you'd prefer more synchronous communication while troubleshooting, I'm always available on the project Discord: discord.gg/8Jzqu3T

Sure, I'll join 👍

@vladimyr
Copy link
Member Author

Well, this reached a dead end 😞

The main difference between Homebrew and pip/x install of xonsh is that Homebrew installs optional prompt-toolkit too. Without it, xonsh simply uses readline and works fine with ANSI escape sequences. With prompt-toolkit in use, things go wrong xonsh/xonsh#1403 so even if I figure out import error there is still a bigger wall to hit. With that being said and because I'm not a python expert and don't have enough time/knowledge to dive deep into xonsh/ptk internals I think it is fair to give up here. I'm sorry, I hope I put up a good fight ⚔️

@anki-code
Copy link
Contributor

I've ported starship as xontrib-prompt-starship

@vladimyr
Copy link
Member Author

vladimyr commented Mar 8, 2021

The main difference between Homebrew and pip/x install of xonsh is that Homebrew installs optional prompt-toolkit too. Without it, xonsh simply uses readline and works fine with ANSI escape sequences. With prompt-toolkit in use, things go wrong xonsh/xonsh#1403

This is not true anymore. Xonsh now properly supports escape sequences with prompt-toolkit being used as demonstrated here: #272 (comment)

Kudos to @anki-code for following xonsh/xonsh#1403 and bringing this to my attention 👍

Thus restarting this effort...

@vladimyr vladimyr reopened this Mar 8, 2021
@andytom andytom mentioned this pull request May 14, 2021
4 tasks
@djmattyg007
Copy link

What would it take to get this PR merged? 🙏

@jeremyschlatter jeremyschlatter mentioned this pull request Jun 19, 2021
5 tasks
@jeremyschlatter
Copy link
Sponsor Contributor

I fixed the merge conflicts and the NameError in a new PR: #2807

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.

Support for the xonsh shell
5 participants