-
-
Notifications
You must be signed in to change notification settings - Fork 543
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 *Nushell* support #788
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
9fcd13d
to
5fa7ca9
Compare
I'll give this a proper test and review tomorrow! Thank you 🙏 |
5fa7ca9
to
1f1684a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of comments - I'm going to install nu now as well, just to get a bit of history
1f1684a
to
1cf7ab5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. I'll have a test soon and see how it feels. Thank you so much ❤️
1cf7ab5
to
884c4fc
Compare
Rebased due to Add history deletion by ellie · Pull Request #791 · ellie/atuin. |
Testing this on my machine atm - it looks like perhaps new history items aren't being stored OK? I'm running commands and nothing is stored, though I am occasionally seeing blank entries. Perhaps the precmd is running OK but post command is not? I'm running And also, the up arrow binding is not bound by default - would be awesome to get the behaviour matching our defaults elsewhere! Otherwise everything is super smooth, thank you! |
Oh you need 0.77.2 as don't think 0.77.1 includes my hook PR. Sure I can include the up arrow keybind too. Good idea. |
Ah! That explains it 😂 I'll sort that now! |
I don't actually think getting the edit buffer is currently possible and pass it to Atuin, due to the Nushell having trouble piping
So can't bind the up arrow key. This PR now just allows searching in Atuin, without getting input from the edit buffer. |
884c4fc
to
45b54b0
Compare
Actually let me see if I can hack around this a bit. Maybe I can use Sh to swap stdout and stderr. |
This seems doable, but an even bigger problem is that I'm not sure how and if it's possible to get the current input buffer's contents, in a |
45b54b0
to
82931b5
Compare
82931b5
to
04c06d1
Compare
Requires my PR nushell/nushell#8560. |
59833d0
to
a71e582
Compare
4f997c9
to
5abbcec
Compare
Hey @stevenxxiu! Is this ready for a review? 🙏 I'll need to run NuShell |
@ellie Yup it should be. I'm guessing Nushell |
Hi! I'm one of the nushell maintainers. I'm excited to use nushell with atuin. However, it breaks my heart a little to see it not be cross platform because of this one part.
Can you help me understand the technical reason why this needs to happen? I'm not a regular bash user but I think this is swapping stderr and stdout, but tbh, I'm not positive. Is there something that nushell could do to help make this work better? |
@fdncred Yes this is just swapping stderr and stdout. I believe the interface is on stdout, and the command that's selected goes to stderr. This gets swapped so stdout contains the command that's selected. From my point of view these two issues need to be addressed: |
@stevenxxiu is it also true that if I'll ask around to see if anyone has ideas about how to make this work in nushell (specifically the issues you linked). TBH, I don't think nushell has our stderr, stdout story all sorted out, it's not very user friendly and accessible right now. It's somewhat difficult work. |
I'll test if we can switch them in other shells to make things easier. Edit: looks like we can with zsh and bash and fish |
5abbcec
to
2b11069
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you so much for this!!! 🙏 Really appreciate the work you have done on both sides.
I'm not sure how well this will work on Windows (and on which terminals/setups it will work), but we don't claim solid Windows support either way - especially given how fresh thast support is.
thanks a lot for this, I've just tested this and ran into a small error |
See NuShell support · Issue #687 · ellie/atuin.