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

kakoune: update ui options #3396

Closed
wants to merge 1 commit into from

Conversation

lobre
Copy link
Contributor

@lobre lobre commented Nov 6, 2022

Description

As ncurses was totally removed from kakoune, the 3 following options don’t exist anymore.

However, 3 new options exist now (https://github.com/mawww/kakoune/blob/bfa3c568ccdc39910e5f9a0cbcc73ed05275087a/doc/pages/options.asciidoc)

  • terminal_padding_char
  • terminal_padding_fill
  • terminal_synchronized

I added those in this PR.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.kakoune-{no-plugins,use-plugins,whitespace-highlighter,whitespace-highlighter-corner-cases}.

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

@lobre lobre requested a review from rycee as a code owner November 6, 2022 11:51
@teto
Copy link
Collaborator

teto commented Nov 27, 2022

I would add mkRemovedOptionModule to help users transition

@lobre lobre force-pushed the kakoune-remove-ncurses branch from c4aab61 to 23dc634 Compare December 29, 2022 09:16
@lobre
Copy link
Contributor Author

lobre commented Dec 29, 2022

I have added mkRemovedOptionModule for the options that were removed.

Let me know if you have any comments, or feel free to merge @teto.

@lobre
Copy link
Contributor Author

lobre commented Dec 29, 2022

I can see the format failed in the CI. I tried to format it using ./format, but the output seems off. See:

in {
  imports = [
    (mkRemovedOptionModule [ "programs" "kakoune" "config" "ui" "changeColors" ]
      "Kakoune dropped the dependency to ncurses so this option has been removed.")
    (mkRemovedOptionModule [
      "programs"
      "kakoune"
      "config"
      "ui"
      "wheelDownButton"
    ]
      "Kakoune dropped the dependency to ncurses so this option has been removed.")
    (mkRemovedOptionModule [
      "programs"
      "kakoune"
      "config"
      "ui"
      "wheelUpButton"
    ]
      "Kakoune dropped the dependency to ncurses so this option has been removed.")
    (mkRemovedOptionModule [
      "programs"
      "kakoune"
      "config"
      "ui"
      "useBuiltinKeyParser"
    ]
      "Kakoune dropped the dependency to ncurses so this option has been removed.")
  ];

What am I supposed to do? Format anyway? Thanks.

@rycee
Copy link
Member

rycee commented Dec 29, 2022

@lobre Don't worry, just ./format 😉

@lobre lobre force-pushed the kakoune-remove-ncurses branch from 23dc634 to 3c99baf Compare December 29, 2022 14:51
@lobre lobre force-pushed the kakoune-remove-ncurses branch from 3c99baf to 7346112 Compare December 29, 2022 16:02
@lobre
Copy link
Contributor Author

lobre commented Dec 29, 2022

I don’t really understand the error message reported by CI. Does anybody have a clue? That is the first time that I use mkRemovedOptionModule but something seems wrong with the parent "programs.kakoune.config".

error: The option `programs.kakoune.config' in module `/home/runner/work/home-manager/home-manager/modules/programs/kakoune.nix' would be a parent of the following options, but its type `null or (submodule)' does not support nested options.
        - option(s) with prefix `programs.kakoune.config.ui' in module `/home/runner/work/home-manager/home-manager/modules/programs/kakoune.nix'
        - option(s) with prefix `programs.kakoune.config.ui' in module `/home/runner/work/home-manager/home-manager/modules/programs/kakoune.nix'
        - option(s) with prefix `programs.kakoune.config.ui' in module `/home/runner/work/home-manager/home-manager/modules/programs/kakoune.nix'
        - option(s) with prefix `programs.kakoune.config.ui' in module `/home/runner/work/home-manager/home-manager/modules/programs/kakoune.nix'

Help you be appreciated.

@lobre
Copy link
Contributor Author

lobre commented Jan 11, 2023

Maybe @teto you have an idea about the problem?

Sorry to spam, but I feel that it is unfortunate that this PR is blocked because of this problem, and I struggle to find a solution. I also asked on IRC but haven’t received any help.

@ncfavier
Copy link
Member

ncfavier commented Jan 11, 2023

NixOS/nixpkgs#96006 strikes again.

I think kakoune is a clear example of a module with too many options, and this PR demonstrates why this is a problem.

There are ways to work around the mkRemovedOptionModule problem but they're all quite hacky. I think my preferred solution here would be to drop every option under programs.kakoune.config.ui, and instead make ui a generic attrsOf value for a suitable value type. Users of the deprecated options will get a warning from kakoune anyway, right?

Consider also adding freeformTypes to other submodules so that future new configuration settings can be added without having to change the module (see RFC 42 for more information).

@lobre
Copy link
Contributor Author

lobre commented Feb 9, 2023

Thank you for your comment and help @ncfavier.

Unfortunately, I don’t have time to work on this anymore. I created this PR while testing kakoune and I thought it would be easier to have those options updated.

I understand the problem and I see the path to the solution. But I don’t have a lot of free time at the moment so I think I will leave this for someone motivated to take over. I am using neovim full-time and I am not sure to come back to kakoune.

I am closing this, but feel free to reopen or take over if anyone is interested.

@lobre lobre closed this Feb 9, 2023
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.

4 participants