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 a rule to detect useless computed property key #111

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

Conversation

BuBuaBu
Copy link

@BuBuaBu BuBuaBu commented Apr 10, 2024

No description provided.

@BuBuaBu BuBuaBu force-pushed the useless_key branch 2 times, most recently from b8857cf to 0654717 Compare April 10, 2024 21:55
Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

I appreciate this and even do personally prefer not quoting keys when not necessary, but hove some reservations about making this a default lint. There are likely to be too many cases in the wild where generated code or in contexts where some keys need protection and some don't for this to be appreciated:

local t = {
   ["a.b"] = true -- good
   ["a'b"] = true -- good
   ["a_b"] = true -- why does this necessarily get called 'bad'?
}

Even normally preferring to use literal keys when possible, this lint would make some normal coding patterns require exceptions.

Have you tried running this on any code bases in the wild that use Luacheck already?

@@ -294,3 +295,24 @@ Additionally, separate limits can be set for three different type of lines:

These types of lines are limited using CLI options named ``--[no-]max-string-line-length``, ``--[no-]max-comment-line-length``,
and ``--[no-]max-code-line-length``, with similar config and inline options.

Style issues (6xx)
Copy link
Member

Choose a reason for hiding this comment

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

6xx as a header seems to be in conflict with the 7xx code described in the section

:linenos:

local foo = {
["a"] = 0, -- bad
Copy link
Member

Choose a reason for hiding this comment

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

This is not a "computed property", there is no computation going on here. It is just a quoted value that can be quite handy when Lua can't parse it as a keyword. This is commonly used in generated code when the code generator can't know if the key is going to be an acceptable literal keyword or not.

At the very least calling these "computed" doesn't seem right. And I agree using them is sometimes unnecessary, but I don't think linting them as "bad" is going to be correct all the time.

Style issues (6xx)
-----------------------

Useless computed key (701)
Copy link
Member

Choose a reason for hiding this comment

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

If we do move ahead with this (I have some reservations about it being on by default) then we'd need to call it "unnecessary", not "useless".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants