Added possibility to have multiple finders #47
Conversation
There was a problem hiding this comment.
LGTM. However, you should address comments from review on original PR
#23 (comment)
#23 (comment)
| @@ -244,6 +243,46 @@ private static Pattern compilePattern(PrintStream logger, String regexp) { | |||
| return pattern; | |||
| } | |||
|
|
|||
| /** cut off first item as it is provided with getters */ | |||
There was a problem hiding this comment.
this comment is no longer valid. I was using only list with 0th item as primary text finder. However, now there is primaryTextFinder and list for others.
|
|
||
| // backward compatibility getters below | ||
| // all these getters are there for backward compatibility | ||
| // get first field from list which is used to store original config values |
There was a problem hiding this comment.
same here, no longer valid comment. we're not getting 0th element, but values from primaryTextFinder
There was a problem hiding this comment.
comments removed. Wil now jump to the #23 (comment)
judovana
force-pushed
the
re-multiple-finders-03
branch
from
b4073be
to
8e52f5e
Compare
|
Adapted to pipelines. Now struggling with html view test... not much successfully. |
|
@basil hi! Major issues were fixed. I think you can start with review. |
A promising start. Looks like my first concern from #23 (comment) was addressed. Now we need to address the second and third concerns from #23 (comment) and the concern from #23 (comment). |
|
hi! Thank you for eyballing it. From my point of view only DSL verification is missing. I'm working on that, but am currently stuck on it. It can be seen in dsl dynamic api view, but methods the dynamic api viewer is showing do not have sense. Was Textfinder plgin actually ever working with dsl, after the change from databound constructors to databoundsetters? Was it wolring on all proeject types? |
|
ping please? |
|
@basil Hi! Do you mind to confirm that also third concern is fixed please? Can you please help with the dsl? I spent few days on it, ant it was not working before this changeset too, or I'm doing something really wrong. Please do not let this change to rot, You could see, that there are at least four teams world wide using this feature, and so now pathcing the builds of yours on thirs own. Thanx! |
|
Dear maintainers, collaboration with this project is really painful |
|
Hi Basil! You have forced us to rework the fully backward comaptible PR to this one. And once we did it, and once we fixed even all other nits, you stopped to communicate. Do you have some final resolution? Maybe we should for as jenkins/multitext-finder-plugin? |
|
We have one more improvement in our for. A field, whicch based on regex can change displayName of run. Not sure if you are interesting in any such change. If not, thane probably we really should stay forked. |
|
Ok. We will continue to live in our fork, https://github.com/judovana/text-finders-plugin/ and will release it as separate plugind |
Hi!
This is direct adaptation and rebase to tip of #23 , whic can be closed (unless merged first of course)
I had merged individual commits to single one - it still have sense, and to rebase it one by one was extremely painful.