[Rpm-maint] [rpm-software-management/rpm] Warn if %else is after %else (#649)

[Panu Matilainen]

pmatilai requested changes on this pull request.

Hmm, there seems to be some GH review-tool wonkiness here, I made several comments on the latest round and haven't touched this because those issues haven't been addressed. But now I noticed that those comments appear as "pending" so I guess you haven't seen them at all because GH thinks I haven't actually submitted them. *Sigh*

> +    size_t textLen;
+    const char *text;
+    int withArgs;
+    int isConditional;
+    int wrongPrecursors;
+    const char *info_text;
+} * parsedSpecLine;
+
+static struct parsedSpecLine_s const lineTypes[] = {
+    { LINE_ENDIF,      LEN_AND_STR("%endif")  , 0, 1, LINE_ENDIF, "with no %%if"},
+    { LINE_ELSE,       LEN_AND_STR("%else")   , 0, 1, LINE_ENDIF | LINE_ELSE, "after %%else" },
+    { LINE_IF,         LEN_AND_STR("%if")     , 1, 1, 0, "after %%if"},
+    { LINE_IFARCH,     LEN_AND_STR("%ifarch") , 1, 1, 0, "after %%ifarch"},
+    { LINE_IFNARCH,    LEN_AND_STR("%ifnarch"), 1, 1, 0, "after %%ifnarch"},
+    { LINE_IFOS,       LEN_AND_STR("%ifos")   , 1, 1, 0, "after %%ifos"},
+    { LINE_IFNOS,      LEN_AND_STR("%ifnos")  , 1, 1, 0, "after %%ifnos"},

The %ifs should not have any info text because there are no such error cases, and the texts should be marked for translation. 

Programmatically constructed messages tend to be a problem for translators, might better to have the whole string in one piece, ie "%s with no %s" and "%s after %s" to allow translators to at least reorder the words.

> @@ -462,6 +430,16 @@ int readLine(rpmSpec spec, int strip)
     lineType = parseLineType(s);
     if (!lineType)
 	goto after_classification;
+
+    /* for a conditional check its ordering */
+    if (lineType->isConditional &&
+	(spec->readStack->lastConditional->id & lineType->wrongPrecursors)) {
+	rpmlog(RPMLOG_ERR,_("%s:%d: Got %%%s %s\n"),

While we're changing the message, *please* drop the stupid "Got" from there. It serves no purpose other being an extra hurdle to translators.

> +    size_t textLen;
+    const char *text;
+    int withArgs;
+    int isConditional;
+    int wrongPrecursors;
+    const char *info_text;
+} * parsedSpecLine;
+
+static struct parsedSpecLine_s const lineTypes[] = {
+    { LINE_ENDIF,      LEN_AND_STR("%endif")  , 0, 1, LINE_ENDIF, "with no %%if"},
+    { LINE_ELSE,       LEN_AND_STR("%else")   , 0, 1, LINE_ENDIF | LINE_ELSE, "after %%else" },
+    { LINE_IF,         LEN_AND_STR("%if")     , 1, 1, 0, "after %%if"},
+    { LINE_IFARCH,     LEN_AND_STR("%ifarch") , 1, 1, 0, "after %%ifarch"},
+    { LINE_IFNARCH,    LEN_AND_STR("%ifnarch"), 1, 1, 0, "after %%ifnarch"},
+    { LINE_IFOS,       LEN_AND_STR("%ifos")   , 1, 1, 0, "after %%ifos"},
+    { LINE_IFNOS,      LEN_AND_STR("%ifnos")  , 1, 1, 0, "after %%ifnos"},

Hmm, "with no" could just as well be changed into "before", which makes the messages more in line with each other: this is just about the order of things, so things are either before or after its proper place.

Also, isn't this now missing the else-with-no-if case, or am I just misreading it?

-- 
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/649#pullrequestreview-231615676
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20190513/24d76e40/attachment.html>