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

coala-format: Use IDs #981

Closed
Makman2 opened this issue Oct 11, 2015 · 13 comments
Closed

coala-format: Use IDs #981

Makman2 opened this issue Oct 11, 2015 · 13 comments
Assignees

Comments

@Makman2
Copy link
Member

Makman2 commented Oct 11, 2015

Because the Result class gets refactored and allows to store multiple result positions/ranges, coala-format can't work that easily like before since serialization of lists makes it too complicated.

Everyone needs to stick with coala-json which is anyway more straightforward to use because:

  • Nearly everyone has json support
  • If someone has no json support, it's easily to code that yourself
  • Even humans can read json
  • Since it already exists we remove boilerplate with coala-format
@sils sils self-assigned this Oct 11, 2015
sils added a commit that referenced this issue Oct 11, 2015
coala-format is not compatible with our future plans because results
will get several ranges of source code. Expressing this in a format
string is awful so we want people to use coala-json instead.

Fixes #981
@AbdealiLoKo
Copy link
Contributor

The reason I had proposed coala format earlier was because a lot of linter classes (example atom-linter) uses regex to parse the output.
This was there simply because loads of other linters (all nearly) have it - and thouhgt it would be good to have.

Note : removing this would mean that the current coala-atom plugin stops working.

Can't we simply write a new line for every range mentioned in a result - with the same message and other details ?

@AbdealiLoKo
Copy link
Contributor

It might be worth it to check what pylint does - because it has the code clone thing - and that has 2 different files and 2 different source ranges. So, they must have a solution for this (maybe?)

@sils
Copy link
Member

sils commented Oct 12, 2015

Can't we simply write a new line for every range mentioned in a result - with the same message and other details ?

We could do that. Does that make sense?

...
origin:ClangCloneDetectionBear:file:file0.c:line_nr:1376:severity:3:msg:Code clone found!
origin:ClangCloneDetectionBear:file:file1.c:line_nr:1476:severity:3:msg:Code clone found!
origin:ClangCloneDetectionBear:file:file2.c:line_nr:1480:severity:3:msg:Code clone found!

With that there would be no possibility of telling wether those lines belong together or not. Maybe we could introduce an ID?

...
id:0:origin:ClangCloneDetectionBear:file:file0.c:line_nr:1376:severity:3:msg:Code clone found!
id:1:origin:ClangCloneDetectionBear:file:file1.c:line_nr:1476:severity:3:msg:Code clone found!
id:1:origin:ClangCloneDetectionBear:file:file2.c:line_nr:1480:severity:3:msg:Code clone found!

@sils
Copy link
Member

sils commented Oct 12, 2015

@AbdealiJK @Makman2 what do you think of that id? I like it because people can choose to ignore this complexity. OTOH if they ignore that users might wonder e.g. what the cloned function now is if a plugin decides to ignore that.

@sils
Copy link
Member

sils commented Oct 12, 2015

It might be worth it to check what pylint does - because it has the code clone thing - and that has 2 different files and 2 different source ranges. So, they must have a solution for this (maybe?)

I think they simply put this into the message. The format string supports no such thing: http://docs.pylint.org/output.html

Taking IDs is even better IMO because people can use that to reference between the occurrences and if they don't want to do that they can add the ID to the message. "Code clone found (1)"

@AbdealiLoKo
Copy link
Contributor

I like the id idea, is similar to the serial number bug I had mentioned before.

But how do you make sure the ids don't clash ? Is it set at the end while printing?

-----Original Message-----
From: "Lasse Schuirmann" notifications@github.com
Sent: ‎12-‎10-‎2015 12:25
To: "coala-analyzer/coala" coala@noreply.github.com
Cc: "AbdealiJK" abdealikothari@gmail.com
Subject: Re: [coala] Remove coala-format (#981)

It might be worth it to check what pylint does - because it has the code clone thing - and that has 2 different files and 2 different source ranges. So, they must have a solution for this (maybe?)
I think they simply put this into the message. The format string supports no such thing: http://docs.pylint.org/output.html
Taking IDs is even better IMO because people can use that to reference between the occurrences and if they don't want to do that they can add the ID to the message. "Code clone found (1)"

Reply to this email directly or view it on GitHub.

@sils
Copy link
Member

sils commented Oct 12, 2015

Is it set at the end while printing?

TBH it seems this would be the most secure thing to do. I tried having a static itertools.count() object but if we have multiple process this really gets complicated and would need interprocess communication. I think the performance overhead posed by this is acceptable since we usually don't expect to get thousands of results, if we do the little counter synchronization is neglectable compared to the analysis.

@sils
Copy link
Member

sils commented Oct 12, 2015

As @AbdealiJK pointed out in a private chat: we could use a hash which is very close to unique.

@sils
Copy link
Member

sils commented Oct 12, 2015

@sils sils changed the title Remove coala-format coala-format: Use IDs Oct 12, 2015
sils added a commit that referenced this issue Oct 12, 2015
The ID is needed for coala-format. See discussion on the related bug.

Fixes #981
@sils
Copy link
Member

sils commented Oct 12, 2015

This solution uses hash and produces suitable numbers, see example output http://pastebin.com/mkm4xVBW

@Makman2
Copy link
Member Author

Makman2 commented Oct 12, 2015

This is highly complicated imo
Much more code to maintain but in fact not that much new functionality
@AbdealiJK why is it so complicated to use json as format?

@AbdealiLoKo
Copy link
Contributor

@Makman2 sure, you can revamp the atom plugin code for json. But the reason
I used coala-format is because atom has a nice Linter class which makes it
super easy and uniform. Else a lot of GUI boilerplate will have to be
redone.

On Mon, Oct 12, 2015 at 3:58 PM, Makman2 notifications@github.com wrote:

This is highly complicated imo
Much more code to maintain but in fact not that much new functionality
@AbdealiJK https://github.com/AbdealiJK why is it so complicated to use
json as format?


Reply to this email directly or view it on GitHub
#981 (comment)
.

@sils
Copy link
Member

sils commented Oct 12, 2015

@Makman2 coala-format is currently about 10 NCLOC and it is a very useful interface and many linters provide such a thing. This makes using coala far easier for people because they can adjust the format to something they want and are not limited to what we give them. Thus I really think we should keep it. Having IDs for results is not that bad either, we can use the hash for now, if a cleaner solution comes up we simply write a counter object that has those problems.

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

No branches or pull requests

3 participants