lint: cleanup unused code and redefined locals

[lewis6991]

All the unused code gets reported as diagnostics in Neovim which is a bit distracting. Attempted to clean most of it up here.

[tomlau10]

I oppose to this change 🤔

unused local is never a lint check in this project, and by default its severity is just at hint level. Below shows how vscode handle a hint diagnostic, it is just a simple greyed out effect and not distracting at all:

• the local sp and local undefined are greyed out as they are unused

Moreover even if you can clean up everything in this PR, as the project evolves with more contributors making new contributions, the codebase will become "dirty" again. So this is not too meaningful.

---

in Neovim which is a bit distracting

I don't know much about neovim, but maybe neovim should implement a better support for hint diagnostics? 😂
I just found this issue: neovim/neovim#30444 (which you have left comments in it as well 👀)

And if luals's author(s) agree on allowing unused / redefined in the codebase, then a better solution might be to disable these two diagnostics completely in the .luarc.json 🤔 , instead of introducing _* to be excluded for unused check. I believe that it will just make the codebase becomes inconsistent in no time🤦‍♂️...

[lewis6991]

unused local is never a lint check in this project

This project currently has no automated lint checks. That doesn't mean problems can't be fixed.

it is just a simple greyed out effect and not distracting at all:

Not all editors have the same UI. The only contract is LSP.

I don't know much about neovim, but maybe neovim should implement a better support for hint diagnostics? 😂

Neovim implements hint diagnostics exactly according to protocol.

Moreover even if you can clean up everything in this PR, as the project evolves with more contributors making new contributions, the codebase will become "dirty" again. So this is not too meaningful.

Many of the changes here remove redundancy, and so makes the code easier to read and reason about. That is meaningful. Just because code gets dirty over time, doesn't mean it shouldn't ever be fixed.

I just found this issue: neovim/neovim#30444 (which you have left comments in it as well 👀)

Yes it seems some users of vscode think that every other editor should display diagnostics the same way as vscode does. It would be a shame if this project only accepts changes that make the experience in vscode better, but not other editors.

And if luals's author(s) agree on allowing unused / redefined in the codebase, then a better solution might be to disable these two diagnostics completely in the .luarc.json 🤔 , instead of introducing _* to be excluded for unused check. I believe that it will just make the codebase becomes inconsistent in no time🤦‍♂️...

What would be better would be to run LuaLS's --check feature through its own codebase. Removing checks contradicts goals of this project. I find it quite surprising that you think disabling checks is an acceptable solution.

The point of unused _.* is to indicate to the reader that this piece of state is intentionally not required in the logic that follows. This is opposed to variables that unintentionally become unused when code is modified or changed.

[dundargoc]

I oppose to this change 🤔

Because? It helps to explain you reasoning so that people have a chance to understand and/or change their approach to their PR. Even without bringing any editor into the mix, removing unused code is standard practice to keep code clean.

[tomlau10]

I oppose to this change 🤔

Because? It helps to explain you reasoning

My whole comment #2865 (comment) is explaining my arguments, I don't know if you have read it @dundargoc
(I am sure @lewis6991 had read it thoroughly because he is replying my comment point by point, and I am reading it now 👀 )

[dundargoc]

You still have not explained why the changes are bad, just that vscode or neovim has some behavior, which I don't understand the relevance whatsoever. This PR is just a refactor to clean up code. Use whatever editor you want, literally no one cares.

[CppCXY]

Your actions are very meaningful, but making such changes to the current version will only affect the progress of merging after the development of version 4.0 is completed. I think we should consider such matters after version 4.0.

[carsakiller]

I don't see any real downsides from these changes. It appears to me to just clean up the code a bit. I like the addition of using _ to signify a purposefully unused variable, and if we are to enforce some code styles in the future with the new refactor¹, it could help make things clearer.

As far as I am aware, v4 is a complete rewrite, so there wouldn't really be any merge conflicts, there would just be the sort of “new history”.

Footnotes

1. I think it would be a good idea for long-term collaboration and organization. ↩

[tomlau10]

edit: Here comes my further reply. I drafted this comment a couple hours ago, and before I posted it maintainers seems replied just now as well. 👀 If maintainers agree on this change, then I have no more reason to object it 🙂 .

---

Not all editors have the same UI. The only contract is LSP.
Neovim implements hint diagnostics exactly according to protocol.

😮 I just learn this today, so it seems that vscode is the one not following standard 🤦‍♂️
Just like some previous issues indicated:

• Only call workspace/configuration when available #2843 (comment)
• completion returns wrong and according to the spec invalid results #2762 (comment)

---

This project currently has no automated lint checks. That doesn't mean problems can't be fixed.
What would be better would be to run LuaLS's --check feature through its own codebase
I find it quite surprising that you think disabling checks is an acceptable solution.

I agreed. But even then, the default severity check level is warning, that means under normal circumstances LuaLS recommends checking diagnostics with severity >= warning as problems to be fixed: https://luals.github.io/wiki/usage/#--checklevel
And the unused group check by default is hint level only: https://luals.github.io/wiki/diagnostics/#unused
=> This is the reason why I think disabling unused checks is acceptable. 😄
I only meant for unused (or in general those hint level diagnostics) but not others.

---

Many of the changes here remove redundancy, and so makes the code easier to read and reason about. That is meaningful ...
The point of unused _.* is to indicate to the reader that this piece of state is intentionally not required in the logic that follows.

I go for remove redundancy (those unused require), there's nothing to argue 👍 But I hesitate on using _.* to mark unused. Because this introduces a new coding standard which needs consensus. This is not a personal/private project but an open source project, where there can be new contributors at any moment. If there is no corresponding lint check to enforce this, the codebase will become inconsistent in no time (this is what I want to express in my original comment).

You said _.* is to indicate a variable not required in the logic that follows, but what if it is required in the future (when codes are modified to add more features)? Do contributor need to rename the variable and remove the _ prefix? These are all my concerns that this new coding standard will lead to inconsistent in near future. And inconsistency is much worse ☹️, because at that time, this _.* will lose it meaning: a variable with _.* prefix cannot guarantee that it is unused.

---

It would be a shame if this project only accepts changes that make the experience in vscode better, but not other editors.

Every project may have a preferred setup. For example

• an android project might require Android Studio,
• and an ios project might require Xcode.

I don't agree this is a shame if this project only make experience in vscode better but not other editors, as there is no way to make all editors happy. 🤔 Unless in the development guide of this project states that any editor is welcomed, then the codebase / settings in this project should be responsible for making every editors happy.

---

In short, I go for fixing anything >= warning, and adding --check in project's CI. 👍 But at the moment I cannot agree on introducing _.* for unused variable. It's just a decision made too hastily without consensus from LuaLS authors. 😕
edit: maintainers seems agreed on introducing this coding style already 🙂

And sorry if I cannot express myself concisely, english is not my first language 🙈

[tomlau10]

You still have not explained why the changes are bad ... Use whatever editor you want, literally no one cares.

I hope my recent reply above can explain myself more clearly @dundargoc 😃
And I am sorry if you found my previous reply offensive. I didn't mean it.

[sumneko]

Unrelated to lint, I believe that cleaning up unused variables and the code itself is an improvement.

[tomlau10]

I admit that I didn't read the whole PR changes initially when I made my first comment 🙏
and clearing redundant variables and unused requires are surely improvements 👍

So my only concern left, is the introduction of _.* for marking (currently) unused variables. I have already expressed my concerns above that I think it will lead to inconsistency in the long run. If no one thinks this will cause issues, then I will humbly accept it. 😃