First pass at removing unneeded capture groups #838
Conversation
|
Part of #839 |
|
Thank you for the PR, @fgsch. I think we have not talked about the unneeded capture groups before. However, I see it loosely related to the rule cleanup project outlined in #808. Is that correct and should we tag this PR and the new issue that you made with that rule cleanup tag. Also: Could you describe the steps and techniques you use in this PR in a few words. Not everybody is a regex king... |
|
It's indeed loosely related but I decided to handle this separately since the process is somewhat manual. I'm happy for you to add the rule cleanup tag as long as we treat them as separate things (e.g. they don't block each other). Regarding the steps and techniques, I will address it in a separate comment. |
|
OK. Back to this one. It was all nice and beautiful, but we let it lay around too long and now it has settled some dust. I'm ready to merge, but we have merged the whitespace PR to v3.0/dev in the meantime. And I also think this should go against v3.1/dev (and later considered for backporting). When rebasing against v3.1/dev it might be worth considering the parallel whitespace PR coming out of rule cleaning as well. This is meant to be pushed in quickly too. If that's settled, I'm ready to merge. |
|
OK, I will rebase this against 3.1 and update this PR. |
|
Thank you. |
fgsch
force-pushed
the
fgsch/rm-unneeded-capture-groups
branch
from
b9c2862
to
01fc636
Compare
|
Updated. I've split the changes across 2 commits, one for the removal of capture groups, and one for the capture action. As I mentioned this is the first of more commits to address it. |
fgsch
force-pushed
the
fgsch/rm-unneeded-capture-groups
branch
from
01fc636
to
95ca50e
Compare
fgsch
force-pushed
the
fgsch/rm-unneeded-capture-groups
branch
2 times, most recently
from
e479a8f
to
72cfe30
Compare
|
Updated after the merge of #897. |
|
Hope to find time to look into this (and many other PRs) today. |
|
I took a closer look and it looks quite good. However, I think some of the capture actions you remove are necessary. For example in 920420, where the match is referenced as TX:0 in the chained rule. |
|
You are correct. I got confused with MATCHED_VAR, sorry. |
|
Please do. And please check the other ones as well. I am not sure it all caught my attention. |
fgsch
force-pushed
the
fgsch/rm-unneeded-capture-groups
branch
from
72cfe30
to
46bd0e2
Compare
|
I've removed the commit altogether for now so this only deals with the capture groups (and the trailing space for travis' sake). |
|
OK. I looked again. No hard regressions seen anymore. However, I am really afraid of killing functionality in areas where we do not have unit tests. This is a fairly dangerous PR. Could somebody else take a close look too? Also, is this meaning to do away with every unnecessary capture group or only some of them? |
All of them. |
|
I'm not really sure I look at the right things, but how about 942440? |
|
Other than that, I have ran extensive tests on this PR and it looks OK AFAICT. I am inclined merge after some more clarifications. Anybody disagree? |
|
Visually walked through this PR and done some very quick tests, I see no problems! |
|
OK, I'll wait for a response on 942440 from @fgsch and then I'll merge. |
|
Re 942440, I have misunderstood your question. This work is meant to eventually do all of them but this PR only covers some. |
|
So we can merge this, but then there will be additional PRs? (-> OK with me) |
|
@dune73 yup, that's the idea. |
|
Thank you, @fgsch. Merging. |
While here also remove some unnecessary capture actions.