[Rpm-maint] [rpm-software-management/rpm] implement %elif (#613)

[pavlinamv]

pavlinamv commented on this pull request.



>  	spec->readStack = rl;
 	spec->line[0] = '\0';
+    } else if (isElif) {
+	spec->readStack->reading = match && spec->readStack->readable;
+	if (spec->readStack->reading)
+	    spec->readStack->readable = 0;
+	spec->line[0] = '\0';
+	match = -1;

> I wasn't asking for detailed explanation against my remarks, just pointing (partial) overlap and throwing possible ideas to explore. 

I think that if you had a concrete remark and I am not planning to involve it in next version of PR, then it is fair to explain why. And usually it is better to explain it precisely.

> Like said: knowing what's involved and the surrounding code, rethink from scratch and see what comes up, you can almost  always do better the second or even third time around. 

I rethink this code many times and for me it results in the refactorization patches before the "elif" patch. 

> You mentioned corner cases that this doesn't take into account - take them into account from the beginning the in the next round.

Yes I mentioned those corner cases. Of course I planned to solve them - but in following PR. There is no gain from solving it in this PR.

> I'm absolutely positive that if-elif-else-endif branching can be expressed and tracked in a more coherent manner than we have here. As discussed in private this ugly code to begin with, take the opportunity to improve the foundation instead of duct-taping over it. Sometimes there's no choice but to duct-tape due to time or API  constraints but neither is an issue here.

If you have some concrete issues or improvements of the PR, please send me them. But I am happy with this code.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/613#discussion_r247311274
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20190112/4c824b56/attachment.html>