Review Request 115014: Make it possible to click on cmake-automoc error

[Kevin Funk]

> On Jan. 14, 2014, 4:08 p.m., Kevin Funk wrote:
> > By the way, the filteringstrateytest needs a major overhaul. Not having the test input inline in the test file is very confusing (cf. testlinebuilderfunctions.h)
> > 
> > Other functions, such as FilteringStrategyTest::testCompilerFilterstrategy don't really check if we've been able to parse the url (and line/col) information. There's FilteringStrategyTest::testExtractionOfLineAndCulmn, which handles that, but only for a subset of the possible cases.
> 
> Morten Volden wrote:
>     I don't think that there was any deeper thought behind the requirement of both line number and URL. IIRC all the filters where it made sense to make the line clickable had both the url and line number at the time - feel free to change that. 
>     
>     The original idea was that tests are divided so that testXXFilterstrategy-tests only tests regular expressions for a given filter behaves as intended. The test for extraction of line and URL should cover only that. 
>     Looking at it again, I think I agree that it makes more sense to tests extraction of line and URL in the testXXFilterstrategy-tests.  
>     
>     IMHO the splitup of the line builder functions and the test makes good sense. The name of the line builder function usually says what it does, and if I need to study it in more detail, I can hover over it and with one click I'm at the function. I usually find it a pain to navigate in files > 1000 LOC. I'm not overly religious about it though :-)
>     
>

Okay, do you think we can ship this as-is?

Regarding the split-up of line builder functions: "if I need to study it in more detail, I can hover over it and with one click I'm at the function" -> That's exactly what I don't find optimal, IMHO it's much easier to have the input and expected output near each other so you can easily compare what may be wrong. Most if not all of the code could be just a single static string in filteringstrateytest.cpp we can test against.

I'll try to come up with a solution to this if I find the time. I also want to add some other filters that I'm missing quite often, e.g. https://bugs.kde.org/show_bug.cgi?id=329953.


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115014/#review47377
-----------------------------------------------------------


On Jan. 14, 2014, 4:04 p.m., Kevin Funk wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115014/
> -----------------------------------------------------------
> 
> (Updated Jan. 14, 2014, 4:04 p.m.)
> 
> 
> Review request for KDevelop and Morten Volden.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> Make it possible to click on cmake-automoc error
> 
> There's a change needed in outputfilteringstrategies to be able to click
> on error items only containing an url (no line information).
> 
> Morten, any reason you try to enforce having a line number?
> 
> It's still useful, even if we just have the url, imo.
> 
> 
> Diffs
> -----
> 
>   outputview/outputfilteringstrategies.cpp 0dcabd8614dfc160fad612a077150ec297eb3735 
>   outputview/tests/filteringstrategytest.cpp 1177c9aaa32361de5164404639e8d855e53019e3 
> 
> Diff: https://git.reviewboard.kde.org/r/115014/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Kevin Funk
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140116/70842d83/attachment.html>