feat(select): override telescope config

[kkharji]

Support passing extra telescope configurations as well as custom theme function + backward compatibility for current state. So hopefully no breaking changes.

[kkharji]

Thanks for the PR!

Looking at the logic changes, I'm seeing two major pieces.

1. Allowing config.telescope.theme to be a function that returns picker options.
2. Merging all keys in config.telescope into the picker options.

My first question is: you can already pass in a table of picker options in the theme option, so what is it that you are trying to do that you cannot currently do? I don't know what problem this is trying to solve, so I don't know how to structure the feedback (apart from pointing out bugs).

Yah at first I was having in old version and hence the merge of master.

it makes more sense if config.telescope is just a table of configuration without theme key. But saw it may break other setup so hacked this.

[stevearc]

it makes more sense if config.telescope is just a table of configuration without theme key. But saw it may break other setup so hacked this.

I agree that the theme key is not the most clear, with the current functionality, but I don't like the solution of putting everything into the top level config table. If we do this, it takes away the ability to add additional configuration options in the future. For example, if we want to add the ability to customize the sorter (like I mentioned in #22), we would have to put that option in the top-level config. Now it's getting passed in to telescope as picker_opts. This makes changes more difficult because if we add a new key we have to make sure that it doesn't conflict with any existing telescope picker configuration key. It also makes the system more brittle because if telescope adds a new config key that conflicts with one we're already using, it can break everything.

If the main issue is that theme isn't the best key name, what about making it an alias for a picker or picker_opts config option?

[kkharji]

I don't like the solution of putting everything into the top level config table. If we do this, it takes away the ability to add additional configuration options in the future.

Well, as a rule of thumb, specially when integrating extenral plugins, it is always best to just provide the 1-1 interface to customize and modify it

Adding sugar over it will lead to more confusion and unnecessary workarounds, like the theme key right now.

I agree it's less code, maybe pretty too but it constrains advance users who would like to add custom stuff to the picker it self without having to deal with the sugar.

In the example of adding custom sorters, If I'm not mistaken picker_opts can override any default, including telescope's and dresing.nvim, for example preview = false should be in the body of picker.new so it can be override by picker_opts

if telescope adds a new config key that conflicts with one we're already using, it can break everything.

Yes that's why it maybe a good idea to just pass config.select.telescope as picker_opts and not have to deal with breaking changes in telescope core.

So if you like I can just pass telescope.config as is, and have a breaking change notice. again I'd recommend to prevent something like #22 to be requested as feature.

[stevearc]

Sorry, I don't think I made my point clearly and it came across as pushing back on parts of your PR that I actually agree with.

Well, as a rule of thumb, specially when integrating extenral plugins, it is always best to just provide the 1-1 interface to customize and modify it

Totally agree! The theme = 'ivy' configuration was put in as a very simple and easy placeholder, but I always expected it to change as requests came in.

In the example of adding custom sorters, If I'm not mistaken picker_opts can override any default

Yep, also right and this was a bad example on my part. Let me give it another go:

Currently every part of the telescope picker that we might want to configure is configurable via picker options. However, I'm not 100% sure that this will forever be the case. Hypothetically, what if we wanted to provide some alternative default mappings to attach_mappings? That would be enough code that I don't want to ask users to copy/paste it into their configs, so I'd like to expose it as a boolean or enum. But where to put it? This is why I'm opposed to passing config.select.telescope in directly to the picker. If we choose to do that, it doesn't leave any room for non-picker telescope options in the future.

I'm totally on board with passing some other key like config.select.telescope.picker_opts directly in, like we currently do with theme, and I agree that the theme name is not the most clear for its current behavior.

[kkharji]

came across as pushing back on parts of your PR that I actually agree with.

😆 don't worry about what I might think. You are the maintainer and responsible for changes, so it is good practice to argue and seek clarifications.

Currently every part of the telescope picker that we might want to configure is configurable via picker options. However, I'm not 100% sure that this will forever be the case.

As a maintainer of Telescope, I can confidently say that it will stay like this forever, because it is considered a feature and it is by design that way.

Hypothetically, what if we wanted to provide some alternative default mappings to attach_mappings? That would be enough code that I don't want to ask users to copy/paste it into their configs, so I'd like to expose it as a boolean or enum.

I see what you are saying. This similar issue to themes and why we have telescope.themes namespace, if you look at dropdown theme, you basically see table of keys:

{
    theme = "dropdown",
    results_title = false,
    sorting_strategy = "ascending",
    layout_strategy = "center",
    layout_config = {
      preview_cutoff = 1, -- Preview should always show (unless previewer = false)
      width = function(_, max_columns, _)
        return math.min(max_columns, 80)
      end,
      height = function(_, _, max_lines)
        return math.min(max_lines, 15)
      end,
    },
    border = true,
    borderchars = {
      prompt = { "─", "│", " ", "│", "╭", "╮", "│", "│" },
      results = { "─", "│", "─", "│", "├", "┤", "╯", "╰" },
      preview = { "─", "│", "─", "│", "╭", "╮", "╯", "╰" },
    },
  }

So in similar fashion, we can have a name space under dressing.telescope.builtins, e.g.

local builtins = require'dressing.telescope.builtins'
config.telescope = requrie'telescope.themes'.get_dropdown {
    attach_mappings = builtins.smart_mappings -- or have it by default
    sorter = builtins.default_sorter -- or have it by default
    preview = false,
}

So what you do think?

[stevearc]

Thanks for breaking that down! Since it's a design goal of Telescope to configure everything via picker_opts, I think that addresses my concerns.

It looks like the CI is slightly broken atm. Will merge and fix it after.