oh-my-fish / theme-agnoster

MIT License
94 stars 63 forks source link

Add support for the new `nix shell` command. #57

Closed bakhtiyarneyman closed 5 months ago

sn0cr commented 2 years ago

Hi, thank you for your contribution! Before we can merge this, do you have more context what this PR adds and more screenshots how it looks in the shell?

kylosus commented 5 months ago

Wow I was about to create the same exact PR only to see this at the last second.

The issue is that nix shell no longer sets the $IN_NIX_SHELL variable, so the current way of determining if we are inside nix is not working anymore. I'll attach some screenshots of what the PR looks like.

image image

Though the prompt can get a little too verbose image

bakhtiyarneyman commented 5 months ago

@kylosus Indeed, it can get too verbose. I don't drop into ad-hoc shells with this many packages often, because as soon as it gets so complex, I make a flake. But I can imagine other people's workflows might be different. I propose we merge as is, and if turns out people care about it, we can implement trimming past a certain number of packages.

Another issue with the current implementation is telescoping shells. I.e. if you accidentally dropped into a nix shell nixpkgs#foo from another nix-shell -p bar --pure", you'd still seenix[pure], which is misleading. We could work around it by checking the$PATHvariable first and falling back to$IN_NIX_SHELL` second (which is an inverse of what this PR does today).

UPDATE: I forgot that I had already implemented using variable max_package_count_visible_in_prompt as a means of controlling the number of packages printed in the shell. The default is 10, we can tweak that.

bakhtiyarneyman commented 5 months ago

@sn0cr Here is the screenshot you asked for! Do you need anything else to merge?

image

kylosus commented 5 months ago

Thanks for all the work!

I was referring to the long package name for neofetch in my last screenshot as being too verbose. I'm not too familiar with nix so I couldn't say if having that whole string in path is normal or not, but it nonetheless seems like a single package can take up a lot of space in the prompt. Do you think it makes sense to add a variable to limit the prompt to a specific number of characters in addition to limiting the number of packages displayed? Or maybe extracting the actual package name from the long version string would be a better option?

Also, can we put the Nix icon nerd fonts glyph (nf-md-nix) in the prompt? I think it would look rather cute.

bakhtiyarneyman commented 5 months ago

For sure!

I just pushed some code to take care of unstable versions and duplicates. I punted on the glyph proposal because I never worked with those.

kylosus commented 5 months ago

Was thinking of something like this image

Or maybe image

Maybe even image

bakhtiyarneyman commented 5 months ago

I like the first one! It's also the only one that's consistent with the other virtual environments.

sn0cr commented 5 months ago

Thank you @bakhtiyarneyman, I really like your contributions. I would merge #56 first and then this one? (In the mean time I also started using Nix, so now I even more understand what you contributed)

bakhtiyarneyman commented 5 months ago

SGTM and thank you for your kind words!