Addition of php script output patterns. Initial development.
Needs ReviewPublic

Authored by santilin on Jun 24 2019, 4:31 AM.

Details

Reviewers
pprkut
Group Reviewers
KDevelop
Summary

Added scriptDir to ScriptErrorFilterStrategy constructor to append it to relative paths
Added indicators to tell between error, warning and notice messages

ScriptErrorFilterStrategy: Cleanup prior to submitting the patch

Test Plan

I work on a daily basis with PHP projects and I'm testing all their outputs items

Diff Detail

Repository
R32 KDevelop
Branch
script_error_php_santilin
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15688
Build 15706: arc lint + arc unit
santilin created this revision.Jun 24 2019, 4:31 AM
Restricted Application added a project: KDevelop. · View Herald TranscriptJun 24 2019, 4:31 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
santilin requested review of this revision.Jun 24 2019, 4:31 AM

This patch adds some patterns to ScriptErrorFilterStrategy to handle some PHP errors so that you can click on the message and the matching file is opened.

Some comments:

  • Shouldn't the patterns be added by the kdevelop-php plugin
  • I needed to know the script working dir in order to handle relative paths. I have hacked it reusing the d->m_buildDir member of OutputModel, but there might be another solution.
  • I am not confident that my usage of QDir, QUrl and QFileInfo are the best ones, but the way I have coded it works.
  • Probably, the ScriptErrorFilterStrategy::scriptDir member should be moved to a new ScriptErrorFilterStrategyPrivate class to mimic the structure of CompilerErrorFilterStrategy (if needed at all)
  • I wonder if testing for empty lines (or shorter than a given lenght) to avoid checking the patterns would be an improvement
  • I've set all the items as Informative when there is a match, so I only need to set indicators for error and warnings
santilin updated this revision to Diff 60549.Jun 24 2019, 4:50 AM

ScriptErrorFilteringStrategyFix: all the items where set as InformativeItem even when there were no matches

Hm, seeing all this hardcoded code for optional plugins, this calls out for someone to look into making this something pulled in from the language plugins instead :) Not your fault, just mentioning the obvious. So should be fine to just add here.

Some quick principal API/ABI comments added, otherwise added @pprkut as PHP contributor to give this a look, not anywhere my personal domain :)

kdevplatform/outputview/outputfilteringstrategies.h
84 ↗(On Diff #60549)

Make constructor explicit, scriptdir -> scriptDir.
To improve over the existing code, please add some API dox what value exactly is expected with th argument scriptDir, not immediately obvious to me (as non-.script code developer).

90 ↗(On Diff #60549)

For consistency, please add instead a pimpl

const QScopedPointer<class ScriptErrorFilterStrategyPrivate> d;

similar to CompilerFilterStrategy

santilin updated this revision to Diff 64717.Aug 27 2019, 9:03 AM
santilin marked 2 inline comments as done.
  • Created private implementation of ScriptErrorFilterStrategy
  • executescript: fix setting working directory to OutputView
santilin updated this revision to Diff 64718.Aug 27 2019, 9:59 AM
  • outputfilteringstrategies: Removed unused include KDir

I agree that conceptually this should be moved to (or supplemented by) the actual language plugins, but also agree that this might be out of scope for this change.
As far as the PHP changes are concerned this is fine with me. However, I'd prefer those patterns to be unit tested and not just documented in comments.
Could you have a look at adding those tests?