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

helix: add extraConfig option #6575

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nouritsu
Copy link

@nouritsu nouritsu commented Mar 6, 2025

Description

Add an extraConfig option for Helix. Fixes #6513

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all
    or nix build --reference-lock-file flake.lock ./tests#test-all using Flakes.
    NOTE: I am assuming the tests pass, as I am facing an unrelated error on both of my machines. All the tests in the actions pass.

  • 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.

Maintainer CC

@nouritsu
Copy link
Author

nouritsu commented Mar 6, 2025

This will be my very first contribution to the Nix community. I am still working on my changes and hence have made a draft PR. I am now searching for a way to append the option I just added to the config file.

@nouritsu nouritsu force-pushed the helix_extra_config branch 5 times, most recently from 1509efe to d3eb8a5 Compare March 6, 2025 21:52
@nouritsu
Copy link
Author

nouritsu commented Mar 6, 2025

Ok this looks messy, but I don't know how to ammend commits without using --amend and then force pushing it.

@nouritsu
Copy link
Author

nouritsu commented Mar 6, 2025

The problem I am facing is that source expects a file path which is given to it by tomlFormat.generate. I need to figure out a way to append the string extraConfig to the contents of this file. It cannot be done before (it cannot be passed through builtins.fromTOML) since then the order of the keys is not preserved, defeating the entire purpose of this PR.

I can think of the following approaches -

  • The config file's contents will be constructed as a string - the settings attribute set will be converted to a TOML string and then a \n character and extraConfig will be appended to that string. This string will then be written to a file in the nix store and given to source.
  • The config file is created using tomlFormat.generate and then read into a string, manipulated and written to another file in the nix store.

The first approach has been implemented.

@nouritsu nouritsu force-pushed the helix_extra_config branch 2 times, most recently from 6bb1c9c to f948e30 Compare March 6, 2025 22:20
The extraConfig option can be used to append
ordered lines to helix configuration. Helix
depends on order for rendering minor mode menus.
@nouritsu nouritsu force-pushed the helix_extra_config branch from f948e30 to 1a5a1ed Compare March 6, 2025 22:32
@nouritsu
Copy link
Author

nouritsu commented Mar 6, 2025

Okay so, unrelated tests seem to be failing on my machine.

╭[aneesh@wsl] // ~/code/home-manager // [+0 -0] on  helix_extra_config                                                                                                       (Thursday) 22:41 // took 7m21s385ms
╰─( )❯ nix-shell --pure tests -A run.all
error:
       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:34:12:
           33|
           34|   strict = derivationStrict drvAttrs;
             |            ^
           35|

       … while evaluating derivation 'nmt-run-all-tests'
         whose name attribute is located at /nix/store/b19xi89pgm1arig3l9xqn1h81zjd1j91-source/pkgs/stdenv/generic/make-derivation.nix:375:7

       … while evaluating attribute 'shellHook' of derivation 'nmt-run-all-tests'
         at /nix/store/fm1dwqb08lxr1gk02niyvb1j7v4181c0-source/default.nix:38:41:
           37|   runShellOnlyCommand = name: shellHook:
           38|     pkgs.runCommandLocal name { inherit shellHook; } ''
             |                                         ^
           39|       echo This derivation is only useful when run through nix-shell.

       … while evaluating the option `nmt.result.success':

       … while evaluating definitions from `/nix/store/fm1dwqb08lxr1gk02niyvb1j7v4181c0-source/nmt.nix':

       … while evaluating the option `home.activation.installPackages.data':

       … while evaluating definitions from `/home/aneesh/code/home-manager/modules/home-environment.nix':

       (stack trace truncated; use '--show-trace' to show the full, detailed trace)

       error: attribute 'applicationName' missing
       at /nix/store/b19xi89pgm1arig3l9xqn1h81zjd1j91-source/pkgs/applications/networking/browsers/firefox/wrapper.nix:167:23:
          166|         inherit icon;
          167|         desktopName = browser.applicationName;
             |                       ^
          168|         startupNotify = true;

Above is output of nix-shell --pure tests -A run.all

@nouritsu nouritsu marked this pull request as ready for review March 6, 2025 22:58
@nouritsu
Copy link
Author

nouritsu commented Mar 6, 2025

How? The tests pass here but not on either of my machines?

@nouritsu
Copy link
Author

nouritsu commented Mar 6, 2025

This completely works on my machine, I used my home manager fork as a flake input and here is an example of me using extraConfig to create an ordered paste minor mode menu.

image

Prior to the introduction of this option, the order of the commands would be sorted alphabetically.

image

@khaneliman khaneliman changed the title add extraConfig option helix: add extraConfig option Mar 9, 2025
@nouritsu
Copy link
Author

Are any changes to be made? If not can we merge?

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.

New Option for Helix Text Editor: programs.helix.extraConfig
1 participant