Re: [Rpm-maint] [rpm-software-management/rpm] Codify built-in macro argument acceptance (#853)
Merged #853 into master. -- 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/853#event-2657687248___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Codify built-in macro argument acceptance (#853)
The PR looks good to me. -- 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/853#issuecomment-534511744___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Codify built-in macro argument acceptance (#853)
After poking around a bit, I ended up dropping the argument length checking entirely: it's not an *error* to echo an empty string, and similarly it is not an *error* to ask for dirname of an empty string. And so on - seems better left alone in reality. On-disk filenames cannot be empty so those cases could raise an error, but leaving that classification effort to some other day. -- 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/853#issuecomment-534467660___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Codify built-in macro argument acceptance (#853)
Yup, the thing with g vs gn is that g reflects whether ':' is there or not, and
gn reflects whether there's something after it: macros that do not accept
arguments must not have g, and macros that do need to have both (but non-zero
gn implies non-NULL g).
And yeah, ```%{echo:%{nil}}``` would work and that's perfectly fine. Which is
exactly the reason that makes me wonder if empty argument should be permitted
for echo, warn and error.
Good point on doFoo() expansion, but note that doFoo() is not the only function
of that kind. Perhaps the central check should be only for g (ie ':') and only
check for the non-empty argument in the places that actually care. Which
probably would leave doOutput() naturally out of the argument checking - making
output of empty string does feel kinda wrong.
I'll change that at least to see how it'd look like. Thanks for reviewing!
--
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/853#issuecomment-534410176___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Codify built-in macro argument acceptance (#853)
No objections from my side. Note that `%{echo:%nil}` would still work. I guess
you tested `gn` instead of `g` on purpose?
You may also want to add a test in doFoo so that this also gets checked after
expansion. Or maybe only check this in doFoo?
--
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/853#issuecomment-534151499___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Codify built-in macro argument acceptance (#853)
Note that while for most built-ins such as %{getenv:...} a non-empty argument
is clearly required, but there are some cases where an empty argument could be
allowed, such as an empty %{echo:} which this turns into an error. I could be
convinced to add a third form where the argument is optional.
Thoughts?
--
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/853#issuecomment-534109610___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
[Rpm-maint] [rpm-software-management/rpm] Codify built-in macro argument acceptance (#853)
Built-in macros either take arguments via %{foo:...} or dont, raise
errors on unexpected and missing arguments.
You can view, comment on, or merge this pull request online at:
https://github.com/rpm-software-management/rpm/pull/853
-- Commit Summary --
* Codify built-in macro argument acceptance
-- File Changes --
M rpmio/macro.c (62)
M tests/rpmmacro.at (21)
-- Patch Links --
https://github.com/rpm-software-management/rpm/pull/853.patch
https://github.com/rpm-software-management/rpm/pull/853.diff
--
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/853
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
