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

Support old option names and don't deprecate them #1127

Closed
wants to merge 2 commits into from

Conversation

hazen2215
Copy link

  • Revert "support old option names but mark as deprecated"
  • Support old option names and don't deprecate them

Changing option names is understandable because it may be useful for writing lua plugins but deprecating old names and adding new codes to detect deprecated options seems unnecessary to me.
vis supports alternative option names and the second commit of this pr just adds old names to alternatives.

I will squash two commits if desired.

@rnpnr
Copy link
Collaborator

rnpnr commented Sep 8, 2023

Why? If it was up to me I would delete all the aliases. These ones are
particularly useless in my view.

Also only 3 lines of code were added. The rest will be deleted after
0.9 is released.

@hazen2215
Copy link
Author

hazen2215 commented Sep 8, 2023

Noticed the deprecation message of 'show-eof' option when using vis on master branch because I have vis:command('set show-eof off') on my visrc.lua.
Until most distros update vis to 0.9 and I can finally use vis.options.xxx, I'm stuck with :set command and seeing such message every time is quite annoying
Also I haven't see config breaking change like this on vis other than scintilua update, but that only broke local lexers and syncing visrc.lua among different versions of vis had previously no problems.

@rnpnr
Copy link
Collaborator

rnpnr commented Sep 8, 2023

If you are seeing that message you can use the supported name instead.

Also I haven't see config breaking change like this on vis other than scintilua update

Hence why it is being marked as deprecated and supported for a
release. Those options were added before vis had a lua interface and so
the names were chosen without support for lua in mind.

@hazen2215
Copy link
Author

OK I found the plugin (https://github.com/milhnl/vis-options-backport) backporting vis.options to older versions that addresses renaming problem.
So if the maintainers want to I'm ok with closing this PR since the solution is found.
But also I'd like to hear others' opinion if consistency is good reason for deprecation and opinion about deprecating something that's been around for some time.

@hazen2215
Copy link
Author

also quote
#1001 (comment)

It will be a very slow, careful process. We don't want to make things worse, and we don't have the intricate knowledge of the internals that martanne had. Please do be patient as we learn the ropes.

@TwoF1nger
Copy link

But also I'd like to hear others' opinion if consistency is good reason for deprecation and opinion about deprecating something that's been around for some time.

vis is hardly a household name. I'd say break everything while you can and don't even bother with deprecation notices.

@hazen2215
Copy link
Author

Whether vis is "hardly" household name with 4K stars is debatable but vis is certainly well known to people who prefer simplicity or "suckless" or "KISS" or whatever
Lately vis is getting lots of commits by incorporating many old PRs since new maintainers are added and while getting the bugs fixed is nice I've seen some projects start making many breaking changes after BDFL(s) disappearance/handover (like anything.el/helm) and I'm concerned about vis going that way.

@mcepl
Copy link
Contributor

mcepl commented Sep 10, 2023

Lately vis is getting lots of commits by incorporating many old PRs since new maintainers are added and while getting the bugs fixed is nice I've seen some projects start making many breaking changes after BDFL(s) disappearance/handover (like anything.el/helm) and I'm concerned about vis going that way.

Quite certainly we will make changes which martanne would not make, for one, we don’t have a crystal ball to read his mind. However, there are some reasons I don’t expect any radical changes in the direction different from what vis does right now:

  1. We don’t have strength to do much. Current rather strong stream of changes is mostly just processing already existing and neglected in PR queue (there are some PRs created by @rnpnr lately, but these are mostly small fixes of existing issues), and I think we just cannot make any radical changes (which is one of unmentioned reasons behind my negative attitude towards suggested changes to treesitter in Better built-in lexers #668). See how much time we need just with updating vis to follow upstream released highlighters from scintillua.
  2. Given that there is nobody with sufficiently large amount of time, there is nobody who can claim to be BFDL, so there is nobody with large enough authority to decide about radical changes. Decision “to do whatever we were doing before” is usually the only one we can agree upon when dealing with any conflict (a possible conflict, I don’t think we had some great conflict so far).
  3. I don’t actually see any reason why to make radical changes in vis. The idea of small C editor with large number of Lua plugins (which anybody can create and modify as they wish them to be) seems like a good design, and I don’t think there is anything which need to be changed.

@rnpnr
Copy link
Collaborator

rnpnr commented Sep 11, 2023

+1 to what @mcepl is saying

Since vis no longer has a "BDFL" the review and application of patches
has taken more of a community driven approach. If you are concerned
about the direction vis is going feel free to make your opinion heard
about any patches here or on the mailing list. Any commits that are not
minor are given some amount of time to sit and give people a chance to
comment. That includes major commits by me (the lua option patch sat
around for around a month before I applied it). I have been applying bug
fixes and minor things at a faster rate but they are still being filtered.

In any case this pull request doesn't fix any of this so I think it
should be closed. I don't think keeping such an alias around because
"thats how it used to be" is a good programming practice.

@rnpnr rnpnr closed this Sep 11, 2023
@hazen2215 hazen2215 deleted the old-option-names branch September 13, 2023 15:49
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.

4 participants