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: Add format instead of only prefix for labels #46

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tjdevries
Copy link

I wanted to be able to have closing labels do something like </ $NAME > (or at least play with the idea).

This change should be backwards compat with prefix

@tjdevries
Copy link
Author

Although now I'm kind of thinking, what if you allowed a function to be passed that returned the virtual text chunk (so it could be text + highlights if you wanted to configure that. it would be basically just as easy to implement)

Copy link
Collaborator

@akinsho akinsho left a comment

Choose a reason for hiding this comment

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

@tjdevries thanks for the PR 🙌🏾 , I made a few comments, nothing major. Think we can get away with re-using some existing logic and doing a hard switch here vs. trying to maintain backward compat since this plugin likely doesn't have very many users atm.

Regarding the function option I agree it would be easy enough to add but I'd rather wait to see if enough people end up wanting that, it appears on almost every line so I don't think it needs to be very fancy.

local user_format = rawget(opts, "format")

local prefix = opts.prefix
if prefix and user_format then
Copy link
Collaborator

Choose a reason for hiding this comment

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

The config module actually already has a mechanism for deprecations, you just need to specify what the fallback key is and the message to show the user
https://github.com/akinsho/flutter-tools.nvim/blob/a2a67dae9ff43d95caddf4d68cf5afcd4cb9d65d/lua/flutter-tools/config.lua#L61

)
end

-- A bit complicated, but basically just keeps backwards compat of passing "prefix"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd actually rather break this (mean as that sounds) whilst this plugin is still fairly small/doesn't have that many users so I can keep things as simple as possible. I think if the user has overridden prefix with the deprecation table it should warn them of the change and they can change their config

@tjdevries
Copy link
Author

Well the thing I was thinking for the function was I kind of wanted to be able to do different highlights for the // at the beginning of the line and then something else for the actual closing label. So doing a function might just be easier to make that, rather than having some convluted mess of options.

@akinsho
Copy link
Collaborator

akinsho commented May 16, 2021

That's fair enough, in which case I'd rather approach this by leaving the existing option as is, and offering a second option which is a function that does what you describe.

I'd approach it this way because by far the majority of issues I've seen are from users who are just trying to get to grips with nvim/lua in the first place so I'd definitely rather there be a very simple option much like coc or vscode where all they have to do is change a string and a more advanced option using a lua function for people who want to do fancier things.

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.

2 participants