Re: [gentoo-dev] [PATCH] sgml-catalog-r1.eclass: New eclass to handle SGML catalogs

[Ulrich Mueller]

>>>>> On Wed, 04 Sep 2019, Michał Górny wrote:

> +# Copyright 2019 Gentoo Authors
> +# Distributed under the terms of the GNU General Public License v2
> +
> +# @ECLASS: sgml-catalog-r1.eclass
> +# @MAINTAINER:
> +# Michał Górny <mgorny@g.o>
> +# @AUTHOR:
> +# Michał Górny <mgorny@g.o>
> +# @BLURB: Functions for installing SGML catalogs
> +# @DESCRIPTION:
> +# sgml-catalog-r1 provides postinst/postrm for regenerating
> +# /etc/sgml/catalog to include all installed catalogs.

catalog...catalog...catalog...

Certainly the style could be improved? How about: "This eclass
regenerates /etc/sgml/catalog in pkg_postinst and pkg_postrm."?

> +
> +case "${EAPI:-0}" in

Quotes aren't necessary here.

> +	7)
> +		;;
> +	*)
> +		die "Unsupported EAPI=${EAPI} for ${ECLASS}"
> +		;;
> +esac

This case statement could be more compact (which would be better
readable, IMHO).

> +
> +EXPORT_FUNCTIONS pkg_postinst pkg_postrm
> +
> +if [[ ! ${_SGML_CATALOG_R1} ]]; then
> +
> +RDEPEND=">=app-text/sgml-common-0.6.3-r7"
> +
> +# @FUNCTION: sgml-catalog-r1_update_catalog
> +# @DESCRIPTION:
> +# Regenerate /etc/sgml/catalog to include all installed catalogs.
> +sgml-catalog-r1_update_catalog() {
> +	local shopt_save=$(shopt -p nullglob)
> +	shopt -s nullglob
> +	local cats=( "${EROOT}"/etc/sgml/*.cat )
> +	${shopt_save}
> +
> +	if [[ ${#cats[@]} -gt 0 ]]; then
> +		ebegin "Updating ${EROOT}/etc/sgml/catalog"
> +		printf 'CATALOG "%s"\n' "${cats[@]}" > "${T}"/catalog &&
> +		mv "${T}"/catalog "${EROOT}"/etc/sgml/catalog
> +	else
> +		ebegin "Removing ${EROOT}/etc/sgml/catalog"
> +		rm "${EROOT}"/etc/sgml/catalog &&
> +		{ rmdir "${EROOT}"/etc/sgml &>/dev/null || :; }
> +	fi
> +	eend "${?}"

Using one eend for each ebegin would improve readability. Also, quotes
around $? aren't necessary.

> +}
> +
> +# @FUNCTION: sgml-catalog-r1_update_env
> +# @DESCRIPTION:
> +# Regenerate environment variables and copy them to env.d.
> +sgml-catalog-r1_update_env() {
> +	# gensgmlenv doesn't support overriding root
> +	if [[ -z ${ROOT} && -x "${EPREFIX}/usr/bin/gensgmlenv" ]]; then
> +		ebegin "Regenerating SGML environment variables"
> +		gensgmlenv &&
> +		grep -v export "${EPREFIX}/etc/sgml/sgml.env" > "${T}"/93sgmltools-lite &&
> +		mv "${T}"/93sgmltools-lite "${EPREFIX}/etc/env.d/93sgmltools-lite"
> +		eend "${?}"
> +	fi
> +}
> +
> +# @FUNCTION: sgml-catalog-r1_pkg_postinst
> +# @DESCRIPTION:
> +# Perform catalog post installation tasks.

Sure, what else would postinst do? :)

> +sgml-catalog-r1_pkg_postinst() {
> +	sgml-catalog-r1_update_catalog
> +	sgml-catalog-r1_update_env
> +}
> +
> +# @FUNCTION: sgml-catalog-r1_pkg_postrm
> +# @DESCRIPTION:
> +# Perform catalog post removal tasks.
> +sgml-catalog-r1_pkg_postrm() {
> +	sgml-catalog-r1_update_catalog
> +	sgml-catalog-r1_update_env
> +}
> +
> +_SGML_CATALOG_R1=1
> +fi