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

feat!: change italics configuration for finer control #91

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

Conversation

delafthi
Copy link
Contributor

@delafthi delafthi commented Dec 7, 2022

Instead of just enabling or disabling italics provide a table where the user can enable italics for different keywords.

    italics = {
        enabled = true, -- Enable/disable italics globally
        -- Enable/disable italics on a finer level. Changing settings below will only have effect when 'enabled' is set to true
        attributes = false,
        builtins = true,
        comments = false,
        constructors = true,
        conditionals = true,
        defines = false,
        emphasis = true,
        exceptions = false,
        fields = false,
        functions = true,
        includes = false,
        keywords = false,
        labels = true,
        parameters = true,
        properties = false,
        repeats = true,
        types = false,
        variables = false,
    },

This change breaks old configurations (the old settings just won't have any effect) due to:

  • italic changed to italics, which requires a table now
  • italic_comments merged into the italics table

Copy link
Owner

@andersevenrud andersevenrud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor comments on the structuring and definitions on this new feature.

I'm open to leaving things as a string: boolean table, but I think maybe just a flat set of strings might be nicer.

italic = true,
italic_comments = false,
italics = {
enabled = true,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having enabled inside a table like this together with all of the toggles feels a bit weird. Maybe something like this instead ?

{
  italic = true,
  italics = { 'attributes', 'comments' } -- and so on
}

Copy link
Contributor Author

@delafthi delafthi Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about this approach?

{
  italics = false -- or {} to disable italics
}
{
  italics = { 'attributes', 'comments', ...} -- to enable italics for named keywords
}

If someone doesn't define the italics key in his config it gets extended by the defaults anyway.

One drawback I see, with a list of strings is that if someone wants to add one keyword to the defaults he has to define all the defaults as well. In this case it would be nice to have an append method.

Copy link
Owner

@andersevenrud andersevenrud Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One drawback I see, with a list of strings is that if someone wants to add one keyword to the defaults he has to define all the defaults as well. In this case it would be nice to have an append method.

Indeed. Another alternative to having that string: boolean table could possibly be to have a callback that provides the list of defaults so that you can define like this:

-- No italics
italics = false

-- Just use defaults
italics = true

-- Custom list
italics = { 'a', 'b', 'c' }

-- With defaults
italics = function(def) return vim.tbl_extend({ 'a', 'b', 'c' }, def) end

This would have the benefit of consolidating things as well, but not sure the callback pattern is super intuitive 🤷‍♂️

But I'm open to your initial suggestion. Combined with the map loop it won't be as verbose as initially was.

Copy link
Owner

@andersevenrud andersevenrud Dec 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One drawback I see, with a list of strings is that if someone wants to add one keyword to the defaults he has to define all the defaults as well. In this case it would be nice to have an append method.

This goes the other way as well. If someone wants to disable any of the defaults some definition would have to be added (with the original implementation).

The more I think of it, the more I lean towards having a plain set of names 🤔 Another solution that came to mind for doing appending is having some kind of "magic" string (like _ or _defaults_ or whatever), but I don't like magical things in configurations 😅

Copy link
Contributor Author

@delafthi delafthi Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about a possible solution this weekend. What do you think about something similar to https://github.com/folke/tokyonight.nvim#%EF%B8%8F-configuration?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about late reply @delafthi, the notification slipped my mind.

Something like that would be nice, but I feel like #70 should come first because that would make things a bit nicer to work with in general (and very easy to manage backward compatability just as is here today).

@@ -83,8 +102,24 @@ local function create_arguments(options, alternatives)

local cs = {
underline = options.underline_option and s[options.underline_option] or s.none,
italic = (options.italic == true or options.italic == nil) and s.italic or s.none,
comments = options.italic_comments and s.italic or s.none,
attributes = (options.italics.enabled and options.italics.attributes) and s.italic or s.none,
Copy link
Owner

@andersevenrud andersevenrud Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuation from last comment, this probably also should be put into an italics table which can be mapped to make things less verbose:

-- Revert to defaults if none was configured
local italicDefaults = { 'comments' }
local italicDefinitions = #options.italics == 0 and italicDefaults or options.italics

-- Map the configured names into styles
-- Could probably just be empty and skip the loop even if disabled. Should have same effect
local italics = {}
for _, v in ipairs(italicDefinitions) do
  italics[v] = options.italic and s.italic or s.none
end

-- Put as a custom style table
local cs = {
  italics = italics
  -- ...
}

Which can be used as { comments, c.grayish, c.none, cs.italics.comments }. Even if the italics table does not contain all available names, it should work as expected because nil should get converted to a "none" down the line.

@andersevenrud andersevenrud added the enhancement New feature or request label Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants