public inbox for gentoo-dev@lists.gentoo.org
 help / color / mirror / Atom feed
From: Duncan <1i5t5.duncan@cox.net>
To: gentoo-dev@lists.gentoo.org
Subject: [gentoo-dev] Re: gstreamer eclass review
Date: Sun, 18 Nov 2012 20:45:29 +0000 (UTC)	[thread overview]
Message-ID: <pan.2012.11.18.20.45.31@cox.net> (raw)
In-Reply-To: 1353265590.23950.13.camel@kanae

Gilles Dartiguelongue posted on Sun, 18 Nov 2012 20:06:30 +0100 as
excerpted:

It's admittedly a style thing thus pretty much up to the author, purely
bikeshedding in the original sense of the essay's trivial color choice,
but...

> # Even though xz-utils are in @system, they must still be added to DEPEND; see
> # http://archives.gentoo.org/gentoo-dev/msg_a0d4833eb314d1be5d5802a3b710e0a4.xml
> if [[ ${GST_TARBALL_SUFFIX} == "xz" ]]; then
> 	DEPEND="${DEPEND} app-arch/xz-utils"
> fi

Single-clause conditional tests (without an else clause) such as the above
can be trivially rewritten like so:

[[ ${GST_TARBALL_SUFFIX} == "xz" ]] && \
 	DEPEND="${DEPEND} app-arch/xz-utils"

Two lines (or one if short enough) instead of three and more concise (fi
line omitted entirely, "if" omitted, "; then" replaced with &&).

It's possible to do similar with if/then/else -> &&/||, but that may
require {} for clarity if not bash parsing, and IMO is not nearly as
clear as the more straightforward no-else-clause case.  So if there's
an else clause, I prefer if/then/else; if not, I prefer the && or || logic.

> if [[ ${PN} != ${GST_ORG_MODULE} ]]; then
> 	# Do not run test phase for invididual plugin ebuilds.
>	RESTRICT="test"
> fi

Another example, this one definitely a one-liner (plus comment,
"invididual" typo left intact)...

# Do not run test phase for invididual plugin ebuilds.
[[ ${PN} != ${GST_ORG_MODULE} ]] && RESTRICT="test"

Or, avoiding that ! in the test and changing the && to ||...

[[ ${PN} == ${GST_ORG_MODULE} ]] || RESTRICT="test"

Also, while we're here, "test" is a string-literal with no possibility of
spaces or anything else that could otherwise confuse bash, so needs no
quotes.  And I'm not sure if the != pattern matching trigger was
deliberate or not (see bash's "help [[" output).  Thus...

[[ ${PN} = ${GST_ORG_MODULE} ]] || RESTRICT=test

... or...

[[ ${PN} == ${GST_ORG_MODULE} ]] || RESTRICT=test

... with the single vs. double equals making the string match (single) vs
pattern match (double) explicit.  If the test-internal-not form is
retained, the string match form would be...

[[ ${PN} ! = ${GST_ORG_MODULE} ]] && RESTRICT=test

... while the pattern match form would retain the != that was used
(the distinction being the separating space, != is a pattern match
as is ==, ! = is a string match as is a single = ).


> # added to remove circular deps # 6/2/2006 - zaheerm
if [[ ${PN} != ${GST_ORG_MODULE} ]]; then
> 	RDEPEND="${RDEPEND} media-libs/${GST_ORG_MODULE}:${SLOT}"
> fi

Ditto... Further examples skipped.

> # @FUNCTION: gst-plugins10_src_install gst-plugins10_src_install() {
> 	gst-plugins10_find_plugin_dir
> 
> 	if has "${EAPI:-0}" 0 1 2 3 ; then
> 		emake install DESTDIR="${D}" || die
>		[[ -e README ]] && dodoc README
> 	else
> 		default
> 	fi
> 
> 	[[ ${GST_LA_PUNT} = "yes" ]] && prune_libtool_files --modules
> }

Here (last line before the function-closing "}", as I said above, I prefer
if/then/else where there's an else clause, so wouldn't change the upper
conditional) we see a counter-example, doing it just as I suggested.
The if/then version (keeping function indent) would look like this.

	if [[ ${GST_LA_PUNT} = "yes" ]]; then
		prune_libtool_files --modules
	fi

Of course regardless of that, the quotes around "yes" may be omitted as
it's a string literal.  (And here, the explicit single-equals string-
literal match is used, as opposed to the double-equals pattern match.  Of
course either one would work here as it's a trivial pattern, just noting
it for consistency.)

	[[ ${GST_LA_PUNT} = yes ]] && prune_libtool_files --modules


But as I said up top, that's (mostly, the pattern matching vs string
matching will occasionally bite if you're not on the lookout for it)
trivial style stuff.  You may well prefer the way it is now.  But in that
case, why the counter-example, omitting if/then?  (You may of course
fairly point out that consistency is a trivial style thing too. =:^)

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman



  reply	other threads:[~2012-11-18 20:46 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-11-18 19:06 [gentoo-dev] gstreamer eclass review Gilles Dartiguelongue
2012-11-18 20:45 ` Duncan [this message]
2012-11-18 22:59   ` [gentoo-dev] " Gilles Dartiguelongue
2012-11-19  4:23     ` Duncan
2012-11-18 23:13   ` Alec Warner
2012-11-21 15:27 ` [gentoo-dev] " Tomáš Chvátal
2012-11-21 20:54   ` Gilles Dartiguelongue
2012-12-06  4:01     ` Maxim Kammerer
2012-12-06  8:40       ` Gilles Dartiguelongue

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pan.2012.11.18.20.45.31@cox.net \
    --to=1i5t5.duncan@cox.net \
    --cc=gentoo-dev@lists.gentoo.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox