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

[Michał Górny]

On Wed, 2019-09-04 at 17:20 +0200, Ulrich Mueller wrote:
> > > > > > 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? :)

Exactly.  I was wondering if it would be good style to just skip
documenting this.

>
> > +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

-- 
Best regards,
Michał Górny