feat: Consistent and sensible layout_config #922
Conversation
| generic_sorter = { sorters.get_generic_fuzzy_sorter }, | ||
| prefilter_sorter = { sorters.prefilter }, | ||
| file_sorter = { sorters.get_fuzzy_file }, |
There was a problem hiding this comment.
Stupid question. Do we want to switch to lua fzy as default sorter? Its faster, matches better? I know this is unrelated, currently just thinking about it 🤣
lua/telescope/config.lua
Outdated
|
|
||
| set("border", {}) | ||
| set("borderchars", { '─', '│', '─', '│', '╭', '╮', '╯', '╰'}) | ||
| border = { {} }, |
There was a problem hiding this comment.
Are we doing anything with that. I am currently confused by that.
If this thing is nil we are not calling the local get_border_size in layout_strategies.lua function and if i am using a theme (dropdown) its true, so get_border_size will be called. I can set it to false, obv and that works as expected but maybe we should set it to true on default. And give it a description. Thoughts?!
There was a problem hiding this comment.
I think that this should probably be just a boolean.
I've changed the default to true and added some documentation.
I'm happy with the situation now, but will wait for someone else to "Resolve" once they're happy with it 🙂
There was a problem hiding this comment.
I am not really proud of that review. I nitpicked way to much about comments and documentation. And had not really much to say about code, because its already really really good. Also i need way more time than 90 minutes to get into how the whole math works so i can make any valid comments about that.
The two main things that needs to be fixed is remove duplicated code:
- remove a mysterious new duplicated previewer with new name 🤣
- remove one of the duplicated
bottom_panein layout_strategies
That are the two things that need to be fixed before merge. All other things can be address at a later time. :)
I'll run the PR for the next week and will look for issues
|
@Conni2461 I've 👍 for stuff that I simply agree with and commented where I have more to say. Since we've moved to a new PR, I no longer have write access, so can't implement any of your suggested changes. |
|
Oh dang, I forgot that you wouldn't be able to write to this. Sorry, if I get on an actual computer this week I'll see if I can figure out a way to do that 😂 |
|
@tjdevries i don't see a way to only add him to one branch/PR, so we can either add him as collaborator to the repository or make him a member of the org. Its up to you :) I can add him, if you don't find a PC ;) |
|
Yeah, go ahead and add. 😁 Thanks. |
|
Thanks for adding me to the org :) I've sorted some of the simpler issues that @Conni2461 mentioned. |
|
I've implemented some of the other changes requested now. The only two remaining are:
I will have a think about how best to check |
| vim.api.nvim_err_write(table.concat(messages, "\n \n ") .. "\n \nPress <Enter> to continue\n") | ||
| end | ||
| end | ||
|
|
||
| deprecated.layout_configuration = function(user_defaults) | ||
| if user_defaults.layout_defaults then | ||
| if user_defaults.layout_config == nil then | ||
| log.warn "Using 'layout_defaults' in setup() is deprecated. Use 'layout_config' instead." |
There was a problem hiding this comment.
We currently use both nvim_err_write and log.warn for deprecation messages.
I think we might also use log.info elsewhere (despite our log level being at "warn") and also just straight error.
Do we want to standardise which method we are using?
|
@tjdevries @Conni2461 Also, is there anything else that we want to change in this PR before it is ready to merge? |
|
Yeah we can talk about standardising for error messages and warning but this is out of scope for this PR. For the deprecated.lua file i prefer one way of doing it, i have no preferences in which way. For other deprecated messages we have, for example in I have not looked at it yet again but i didn't have big issues while using it. I 100% prefer to get this merged before neovim 0.5 gets released. It would be awful if new people check this out, have a config they feel comfortable with and we change everything within a couple of days. I could take another look at the source code tomorrow morning :) |
skip-checks: true
skip-checks: true
skip-checks: true
- Move `layout_defaults` handling to `deprecated.lua` - Check for "layout keys" outside of `layout_config` on `setup`
|
I plan on merging this today or tomorrow. :) |
|
@l-kershaw thanks for your work here :) |
Related to this PR: nvim-telescope/telescope.nvim#922 They changed the name for config (I think?), but rest of defaults stayed the same.
Moving to this PR from #823 because of some problems with being able to merge back, but I didn't want to lose the history from that PR.
This should also hopefully make it easier for people to comment on the PR and give us some feedback before merging.
cc @l-kershaw @Conni2461