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(plugins): migrate indent-blankline to v3 #4427

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cpea2506
Copy link
Member

@cpea2506 cpea2506 commented Nov 28, 2023

Description

Update indent-blankline to latest version 3.

Note: still not sure how to handle error message correctly and make it more understandable to users

How this has been test

  • A message will popup to indicate that you are using old config and require you to update to match new version.

    Screenshot 2023-11-28 at 23 06 47
  • Currently there is no specific message from original plugin on how to migrate config exactly.

Copy link
Member

@LostNeophyte LostNeophyte left a comment

Choose a reason for hiding this comment

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

this is a breaking change, if a user edits buftype_exclude or filetype_exclude in their config, it will now cause an error. I'd set them to empty tables in _deprecated.lua and add a metatable with _index and _newindex set to a function that will notify the user of the change

@cpea2506
Copy link
Member Author

cpea2506 commented Nov 28, 2023

this is a breaking change, if a user edits buftype_exclude or filetype_exclude in their config, it will now cause an error. I'd set them to empty tables in _deprecated.lua and add a metatable with _index and _newindex set to a function that will notify the user of the change

Screenshot 2023-11-29 at 00 26 17

I think it can warn user in case, the new indentline will do a validate for all of available config options

@LostNeophyte
Copy link
Member

LostNeophyte commented Nov 28, 2023

in that case just set them to empty tables, unless that also triggers the warning, then we would need to set them to nil if they're empty before passing them to setup()

@cpea2506
Copy link
Member Author

cpea2506 commented Nov 28, 2023

in that case just set them to empty tables, unless that also triggers the warning, then we would need to set them to nil if they're empty before passing them to setup()

Then in that case, user won't know that they have wrong key inside their config.
FYI, the message is from indent-blankline

@LostNeophyte
Copy link
Member

unless they set them to empty tables which i doubt then they'll know. We'll only set them to nil the user left them empty

Copy link
Collaborator

@kylo252 kylo252 left a comment

Choose a reason for hiding this comment

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

isn't it easier to keep the existing config for now and adapt it internally to the new layout? any new options can match the ones upstream.

otherwise, we need to "ignore" the old options but also warn with a deprecation message

@cpea2506
Copy link
Member Author

isn't it easier to keep the existing config for now and adapt it internally to the new layout? any new options can match the ones upstream.

otherwise, we need to "ignore" the old options but also warn with a deprecation message

I need a guides on how to handle this correctly.

What I wonder for now are:

  • We have a little information in the migrate-guide. So if we want to send deprecation message on old option, should we need to create a list of old options and iterate through them (as they won't be changed in the future release)?
  • New ibl will send error to user if they set key that is not available in default options as mentioned here and here. I think it's better to convert the error to warning that point user to their config instead.
    ảnh

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.

None yet

3 participants