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 *Nushell* support #788

Merged
merged 4 commits into from
Mar 26, 2023
Merged

feat: add *Nushell* support #788

merged 4 commits into from
Mar 26, 2023

Conversation

stevenxxiu
Copy link
Contributor

@vercel
Copy link

vercel bot commented Mar 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
atuin ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 26, 2023 at 6:02AM (UTC)

@ellie
Copy link
Member

ellie commented Mar 18, 2023

I'll give this a proper test and review tomorrow! Thank you 🙏

Copy link
Member

@ellie ellie left a 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

README.md Show resolved Hide resolved
atuin-client/src/import/nu_histdb.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@conradludgate conradludgate left a 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 ❤️

src/shell/atuin.nu Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
atuin-client/src/import/nu_histdb.rs Outdated Show resolved Hide resolved
@stevenxxiu
Copy link
Contributor Author

@ellie
Copy link
Member

ellie commented Mar 20, 2023

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 nu 0.77.1 so I think that should be ok

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!

@stevenxxiu
Copy link
Contributor Author

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.

@ellie
Copy link
Member

ellie commented Mar 20, 2023

Ah! That explains it 😂 I'll sort that now!

@stevenxxiu
Copy link
Contributor Author

stevenxxiu commented Mar 21, 2023

I don't actually think getting the edit buffer is currently possible and pass it to Atuin, due to the Nushell having trouble piping stderr:

So can't bind the up arrow key. This PR now just allows searching in Atuin, without getting input from the edit buffer.

@stevenxxiu
Copy link
Contributor Author

Actually let me see if I can hack around this a bit. Maybe I can use Sh to swap stdout and stderr.

@stevenxxiu
Copy link
Contributor Author

stevenxxiu commented Mar 21, 2023

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 executehostcommand. I'm asking Nushell people about this.

@stevenxxiu
Copy link
Contributor Author

Requires my PR nushell/nushell#8560.

src/shell/atuin.nu Outdated Show resolved Hide resolved
src/shell/atuin.nu Outdated Show resolved Hide resolved
@ellie
Copy link
Member

ellie commented Mar 24, 2023

Hey @stevenxxiu! Is this ready for a review? 🙏 I'll need to run NuShell master to test this, right?

@stevenxxiu
Copy link
Contributor Author

stevenxxiu commented Mar 24, 2023

@ellie Yup it should be. I'm guessing Nushell master is the easiest. Personally I'm just running v0.77.2 with a few patches including this one. Thanks! ❤️

@fdncred
Copy link

fdncred commented Mar 24, 2023

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.

def _atuin_search_cmd [...flags: string] {
    [
        $ATUIN_KEYBINDING_TOKEN,
        ([
            `commandline (sh -c 'RUST_LOG=error atuin search `,
            $flags,
            ` -i -- "$0" 3>&1 1>&2 2>&3' (commandline))`,
        ] | flatten | str join ''),
    ] | str join "\n"
}

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?

@stevenxxiu
Copy link
Contributor Author

@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:

@fdncred
Copy link

fdncred commented Mar 24, 2023

@stevenxxiu is it also true that if atuin could control where the output gets places (stdout vs stderr), via a parameter, then there wouldn't be a need for this?

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.

@conradludgate
Copy link
Collaborator

conradludgate commented Mar 24, 2023

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

README.md Outdated Show resolved Hide resolved
Copy link
Member

@ellie ellie left a 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.

@ellie ellie merged commit a7cb21a into atuinsh:main Mar 26, 2023
@stevenxxiu stevenxxiu deleted the feat/nushell branch March 26, 2023 15:31
@happysalada
Copy link

thanks a lot for this, I've just tested this and ran into a small error
#841
happy to provide more feedback and/or test anything.

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.

None yet

5 participants