From: Eric Sunshine Date: Mon, 31 Aug 2015 17:16:23 GMT Subject: Re: [PATCH v14 04/13] ref-filter: implement an `align` atom Message-ID: In-Reply-To: On Mon, Aug 31, 2015 at 5:55 AM, Karthik Nayak wrote: > On Sun, Aug 30, 2015 at 3:10 PM, Eric Sunshine wrote: >> On Sun, Aug 30, 2015 at 9:38 AM, Karthik Nayak wrote: >>> On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine wrote: >>>> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak wrote: >>>>> + } else if (!strcmp(name, "align")) >>>>> + die(_("format: incomplete use of the `align` atom")); >>>> >>>> Why does %(align) get flagged as a malformation of %(align:), whereas >>>> %(color) does not get flagged as a malformation of %(color:)? Why does >>>> one deserve special treatment but not the other? >>> >>> Didn't see that, I think its needed to add a check for both like : >>> >>> else if (!strcmp(name, "align") || !strcmp(name, "color")) >>> die(_("format: improper usage of %s atom"), name); >>> >>> I had a look if any other atoms need a subvalue to operate, couldn't >>> find any. >> >> Hmm, I'm not convinced that either %(align) or %(color) need to be >> called out specially. What is the current behavior when these >> "malformations" or any other misspelled atoms are used? Does it error >> out? Does it simply ignore them and pass them through to the output >> unmolested? > > It just simply ignores them currently, which is kinda bad, as the user > is given no warning, and the atom is ineffective. Warning about unrecognized atoms may indeed be a good idea, however, the current behavior isn't a huge problem since user discovers the error when the output fails to match his expectation. This behavior of ignoring unrecognized atoms predates your work, doesn't it? If so, it's probably not something you need to address in this series.