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

nushell: introduce settings option #3831

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bobvanderlinden
Copy link

@bobvanderlinden bobvanderlinden commented Apr 2, 2023

Currently it is convoluted to set basic configuration options for Nushell. It always requires "let-env config =". Because multiple sources need to set these options from home-maanger, the config needs to be updated as well, resulting in multiple "let-env config" statements that update potentially existing config values.

When combined with other home-manager options that add to the Nushell configuration it is error-prone to set options yourself. New users will want to set "show_banner = false", but it is non-trivial when direnv integration is enabled.

This change makes sure all settings can be set at once. The settings are serialized to Nu code.

In addition, the settings also support Nu expressions, which allows referring to variables set in env.nu as well as using more complex expressions.

Lastly, the test examples have been updated with new (nested) configuration options. See
https://www.nushell.sh/blog/2022-11-29-nushell-0.72.html#config-options-have-been-grouped-together

Description

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

@bobvanderlinden bobvanderlinden force-pushed the pr-nushell-settings branch 2 times, most recently from 4499d29 to 260f0e6 Compare April 2, 2023 14:10
Currently it is convoluted to set basic configuration options for
Nushell. It always requires "let-env config =". Because multiple sources
need to set these options from home-maanger, the config needs to be
updated as well, resulting in multiple "let-env config" statements that
update potentially existing config values.

When combined with other home-manager options that add to the Nushell
configuration it is error-prone to set options yourself. New users will
want to set "show_banner = false", but it is non-trivial when direnv
integration is enabled.

This change makes sure all settings can be set at once. The settings are
serialized to Nu code.

In addition, the settings also support Nu expressions, which allows
referring to variables set in env.nu as well as using more complex
expressions.

Lastly, the test examples have been updated with new (nested)
configuration options. See
https://www.nushell.sh/blog/2022-11-29-nushell-0.72.html#config-options-have-been-grouped-together
@bobvanderlinden
Copy link
Author

@Philipp-M Could you have a look?

@Philipp-M
Copy link
Contributor

I'm a little bit undecided about wrapping/converting nix to nu. At the time I have contributed the current way it's configured, I decided against it, as nu script may evolve and this will very likely increase maintenance burden of this module (e.g. supporting different versions of nushell, adapting to changes of the nu language). I absolutely understand the motivation and like the idea of this change though, to have better interoperability with (other modules of) home-manager/nix, as the current state is not completely optimal. I'm currently not using nushell as daily driver, so others should probably weigh in their opinions/thoughts. If we're proceeding with this, I think it would make sense that you'll add yourself as maintainer as well. And one thing while skimming over it: There should probably an assertion for the nushell version ( >= 0.72) when using the settings, or a warning or something like that.

I'm shamelessly pinging a few people, who are/were at least somewhat involved with nushell+home-manager, maybe they have input as well.
@colemickens, @autophagy, @SaiProton, @Lord-Valen, @teto, @happysalada, @ncfavier, @schuelermine

@bobvanderlinden
Copy link
Author

Thanks for the thoughtful comment, suggestions and shameless pings 😄

I get why this wouldn't be accepted, but with my current configuration file becoming quite messy I thought it was worth a shot. With something like this PR the config.nu becomes a lot more like what users would manually write in their config file. Easier to grasp what is going on, easier for newcomers to add show_banner = false and easier for integrations to interop with each-other (especially when introducing conflicting settings that would otherwise silently overwrite eachother depending on order 😅).

I've incorporated an assertion for nushell >= 0.73 and added myself as maintainer. 👍

@stale
Copy link

stale bot commented Jul 9, 2023

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Jul 9, 2023
@nyabinary
Copy link

Is this uh still dead?

@stale stale bot removed the status: stale label Nov 28, 2023
@bobvanderlinden
Copy link
Author

It's here if we want such a feature. I'm currently not actively using nushell anymore, so it's fine to leave this open or close it. When I was using nushell this helped me disable the banner more easily.

Maybe it helps if you want this feature to give some motivation/reason/usecase to adopt this.

@nyabinary
Copy link

It's here if we want such a feature. I'm currently not actively using nushell anymore, so it's fine to leave this open or close it. When I was using nushell this helped me disable the banner more easily.

Maybe it helps if you want this feature to give some motivation/reason/usecase to adopt this.

Well I want to use it :3 I think that valid usecase enough? @rycee This is good to merge right?

@nyabinary
Copy link

Merge conflict btw :3

@Philipp-M
Copy link
Contributor

I'm happy to review an updated PR, though I'm not able to maintain it effectively, since I'm not using nushell actively currently (new maintainers welcome), so I would rely on issues and/or PRs.

@Lord-Valen
Copy link
Contributor

This won't work with >= 0.83. let-env was removed. We must use $env.config = instead. That should actually simplify the implementation for us, thankfully.

Copy link

stale bot commented Apr 15, 2024

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Apr 15, 2024
@nyabinary
Copy link

@Philipp-M What does this need to be merged :P

@Philipp-M
Copy link
Contributor

Well my last comment is still accurate.

I'm not using nushell currently, and have not a deep insight what needs to be done exactly (and what has changed in nushell since I last used it), but I'm happy to review an updated PR, which someone needs to do (I'm not having the time for that currently).
AFAIK this is a little bit out of date, so if you want to do it, go for it.
I would also be happy, if someone else would (additionally) maintain the nushell module, #5500 may be also relevant.

@stale stale bot removed the status: stale label Jun 10, 2024
@nyabinary
Copy link

Well my last comment is still accurate.

I'm not using nushell currently, and have not a deep insight what needs to be done exactly (and what has changed in nushell since I last used it), but I'm happy to review an updated PR, which someone needs to do (I'm not having the time for that currently). AFAIK this is a little bit out of date, so if you want to do it, go for it. I would also be happy, if someone else would (additionally) maintain the nushell module, #5500 may be also relevant.

Commented this because with the new update nushell broke for me :P #5527

@JoaquinTrinanes JoaquinTrinanes mentioned this pull request Jun 18, 2024
6 tasks
Copy link

stale bot commented Sep 15, 2024

Thank you for your contribution! I marked this pull request as stale due to inactivity. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@stale stale bot added the status: stale label Sep 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants