-
Notifications
You must be signed in to change notification settings - Fork 315
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
add detect_cyclomatic_complexity #141
Conversation
hello @mpeterv , But some test cases call For example:
Would you merge this PR if I fix all failed cases properly? And what's your suggestion about how to fix this issue? best regards |
Hello @xjdrew and thank you for the PR! I agree that having cyclomatic complexity check in Luacheck is useful. I'm not sure yet if it's better to have it enabled by default or not, as it's a less common check and can require some research to fix the warnings. I'll deal with tests failing due to the new warning after merging the PR. However, there should be some new tests ensuring that this new warning is working properly. Would you mind adding some tests in There are also some other minor issues:
I don't mind fixing these issues myself after merging but it may be slower that way. |
local CyclomaticComplexityMetric = utils.class() | ||
|
||
function CyclomaticComplexityMetric:incr_decisions(count) | ||
if self.count then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.count
is always set when this is called, it's a bug if it's not. So it's better to remove this condition IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
-- reset | ||
self.count = 0 | ||
self:calc_stmts(line.node[2]) | ||
local stmts = self.count |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is not used, can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forget to remove test codes....
@mpeterv I have make detect_cyclomatic_complexity disabled by default, and ignore 711 warning in So there is only one case failed by now:
|
@mpeterv by now, only -- can't capture name
function A()
end
lcoal m = {}
-- can't capture name
function m.B()
end
-- can't capture name
function m:C()
end |
src/luacheck/cache.lua
Outdated
@@ -15,7 +15,8 @@ local option_fields = { | |||
"ignore", "std", "globals", "unused_args", "self", "compat", "global", "unused", "redefined", | |||
"unused_secondaries", "allow_defined", "allow_defined_top", "module", | |||
"read_globals", "new_globals", "new_read_globals", "enable", "only", "not_globals", | |||
"max_line_length", "max_code_line_length", "max_string_line_length", "max_comment_line_length" | |||
"max_line_length", "max_code_line_length", "max_string_line_length", "max_comment_line_length", | |||
"max_cyclomatic_complexity", | |||
} | |||
|
|||
local event_fields = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ugly but it's necessary to add complexity
to this list as otherwise the field won't be saved correctly when caching check results.
@xjdrew yes, there should be a way to attach a full name to functions like those. It's not done currently as right now names are only needed for unused local functions. It doesn't have to be done in this PR though. Thanks for adding the tests. Some more things:
|
@mpeterv thanks for you suggestions. in future, we can check more metrics of the code quality, for example max depth, max statments of function and so no; can use code 7xx for this purpose. so I think 7xx is necessary. I'll fix the |
@xjdrew I've pushed your commits to |
@mpeterv when do you plan to merge this feature? |
Sorry for this huge delay @xjdrew =( Merged into master, still want to adjust some things before a release. |
Were there any docs changes merged with this PR? It appears that the feature doesn't appear in the online documentation! |
@LPGhatguy looks like for some reason readthedocs had |
detect McCabe’s complexity, warning if function's complexity > 10