search

rrrene / credo

A static code analysis tool for the Elixir language with a focus on code consistency and teaching.
http://credo-ci.org/
MIT License
4.93k stars 417 forks source link

Include both start and end line_no and column in issue #1047

Closed mhanberg closed 8 months ago

mhanberg commented 1 year ago

What were you trying to do?

Issues include a line_no and column field, but for editors to be able to correctly highlight the problems, they need more information.

You can see here, that only the first character of the code is underlined.

image

You can also see here, where the column information is not correct in general

image

Expected outcome

The issue includes the text document full range of the problem

Actual outcome

The issue includes only the start of the range, and that is not accurate sometimes.

rrrene commented 10 months ago

Hi, I notice just now that I never commented after reading the linked PR's discussion. Sorry for that.

As was discussed in the linked PR, Credo's issues have a trigger field that contains the string causing the issue (e.g. "unless" for the UnlessWithElse check).

Deriving the end_column from that should be trivial, no?

rrrene commented 9 months ago

Checking in 😎

mhanberg commented 9 months ago

Sorry, we just had a baby over here haha.

In the second screenshot the column info is just wrong, so calculating the end column using the trigger isn't going to be accurate. But assuming that is just a bug in certain checks, then yes it can be doable as long as the span shouldn't cover multiple lines.

Should the cyclomatic complexity check highlight the entire case expression or just the word case? For example.

rrrene commented 9 months ago

But assuming that is just a bug in certain checks

Yes. I fixed most of them three weeks back. Will be released soon.

Should the cyclomatic complexity check highlight the entire case expression or just the word case?

Here I think the trigger should always be the function's name as it is the function that is perceived as "too complex".

But CC is a really ... controversial ... check/metric. in the sense that it calls on people to change their flow to satisfy a linter.

I get those warnings for perfectly readable functions and think I would not include it again, if there every came a Credo 2.0.

rrrene commented 8 months ago

But assuming that is just a bug in certain checks

Yes. I fixed most of them three weeks back. Will be released soon.

@mhanberg These fixes are now live in Credo 1.7.4 :tada:

mhanberg commented 8 months ago

Nice!

I'll go ahead and close the issue. Thanks for working through it!