fix-9205: Fill task options explicitly if problemMatchers set #10166
Conversation
There was a problem hiding this comment.
Hi @kostmetallist, thank you for your first contribution.
Please be sure to properly sign the eclipse contributor agreement (eca) with the same email as your authorship so we can accept your changes.
msujew
added
tasks
|
as far as I understand the changes are taken from #9207 |
kostmetallist
force-pushed
the
patch-1
branch
from
6ef4cb4 to
7197384
Compare
|
@msujew @RomanNikitenko thanks for the feedback! I've signed the commit and also have provided a reference to author of the original solution from #9207. |
|
Hey guys, congrats for the great 1.18 release! Is there still anything required for the changes to be approved? |
|
@kostmetallist About
I meant something like that:
|
Signed-off-by: Konstantin Kukushkin <[email protected]> Co-authored-by: Vladimir Fedotov <[email protected]> Originally-proposed-by: Vladimir Fedotov <[email protected]>
|
Hi @RomanNikitenko, I've updated the co-authors for the commit, can you please take a look? |
There was a problem hiding this comment.
How to test
Please see #9207 (comment)
@kostmetallist
What extension/plugin did you use for testing?
I tried to test according to #9207 (comment) and faced with the problem:
Screen.Recording.2021-10-07.at.13.15.36.mov
|
Thanks @RomanNikitenko for pointing out the issue! I've initialized new testing stand: https://github.com/kostmetallist/theia-problem-matcher-stand. Please make sure to run Attaching GIF showing everything's fine with problem matchers (please note I'm running modified Theia including changes proposed in this PR).
|
There was a problem hiding this comment.
Tested according to #9207 (comment) - it works well!
Also tried the changes for typescript and shell tasks - I don't see any regressions!
|
Thanks Roman! How likely the changes are to be included in the upcoming 10/28/2021 release? |
We often times leave PRs open for a few days after approving - assuming there's no urgent reason for merging it - for other reviewers to give their "veto" (i.e. other small issues they would like to see resolved). I'm quite sure that your changes will be included in the next release 👍 @RomanNikitenko You can merge this (as you are the main reviewer) as you see fit. Like we discussed on the recent dev meeting, we try to merge approved PRs in a more timely manner now. |
|
Sorry for the delay - I had a short vacation, merging the PR... |
What it does
Fixes issue 9205. Was originally proposed in #9207 by @vnfedotov. This one resolves conflicts with the stale changes.
How to test
Please see #9207 (comment)
Review checklist
Reminder for reviewers
Signed-off-by: Konstantin Kukushkin [email protected]