Re: view-fuzzy beta version ready

[Yavor Doganov]

В 16:55 +0300 на 31.03.2010 (ср), Yavor Doganov написа:
> I hope to be able to (finally!) review and test it carefully within
> this week.

I'm afraid I still don't understand the script entirely :-(  What
follows is a first-round review mostly regarding stylistic issues.
I am still testing it on various machines and I'm kinda surprised by the
different results/experience I get.  I will do a second review before
installing it in trunk (and when your papers are ready).

General comments:
      * Please wrap long lines to maximum 80 (or better, 79) limit.
      * Improper indentation (I'll include some examples below).
      * Improper punctuation (in both cases "improper" == "not according
        to the GCS").
      * Documenting functions is good, but avoid C types, please, they
        are somewhat confusing when used in shell scripts.  (Examples
        below.)
      * I have not encountered in real life the Bash built-ins readonly,
        declare, local, etc., and I'm not ultimately familiar with their
        semantics.  Is this purely an optimization for Bash?  If so, and
        the script is already bash-specific (which it seems it is even
        if you remove those), then their usage is OK.  Otherwise,
        they're just noise.
      * All comments (top-level or along lines of code) should start
        with a capital letter and end with a period.  If the comment
        involves more than one sentence, there need to be two spaces
        (intervals) between the period terminating the first one and the
        capital letter beginning the next one.

| #!/bin/bash
| # Report fuzzy translations with version diff of msgids of gettext files

An empty line after the shebang, and end the description with a period.
The copyright notice should be the standard GPLv3 one -- 4 distinct
paragraphs, separated by empty commented lines.

| ######## Aknowlegdements ###################################

If you really like these logical separators, please make them shorter
(like 3 `#'s, heading, 3 `#'s).

| readonly APPNAME='view-fuzzy'
| readonly VERSION='0.1.0'
| readonly CR_YRID='2010 Georgios Zarkadas'
| readonly CR_MAIL='georgios.zarkadas at gmail.com'

These have to be taken out entirely and the relevant functions to be
updated to use the literals @PACKAGE_NAME@, @PACKAGE_VERSION@, etc.
which are substituted accordingly when the package is configured and
built by the user.

| readonly ESC_BLD=$(tput bold)
| readonly ESC_FIN=$(tput sgr0)

I have no clue whether this is portable.  It works on oldish GNU/Linux
systems, so I'd assume it is OK.  If we get complaints/reports for other
free systems (like *BSD), it needs to be redesigned/worked around.

I wonder what's stopping you to use the standard wdiff coloring scheme
which is already portable, and IMVHO, more readable?

| declare -A LIDS=(
|       ['-c']=0 ['--colors']=0 ['-k']=1 ['--brackets']=1 ['-s']=2 
['--strong']=2
| )

Excessive indentation here (and almost everywhere).

| # void show_help()
| #
| # Show help screen with usage example and options / arguments.

This is much more clearer and GCS-compliant when written as:

# show_help
#
# Short description what the function does.

or

(if the function accepts arguments)
# show_help FOO BAR BAZ

and/or

(if applicable:)
# Arguments:
#  FOO     Some foo option;
#  BAR     Baring option;
#  BAZ     Cool bazzling option.

This is how it has been done traditionally (more or less) within the GNU
project.  I realize it is just a matter of preference, not necessarily
coinciding yours (which is OK), but we try to stick to the common
standards, it helps a lot when exchanging code or even when a particular
person contributes to another GNU project.

|       echo $(mktemp --tmpdir="${TEMP_BASE}" --quiet "${TEMP_TPL}")

These are coreutils-specific options.  mktemp was recently added (circa
2007, IIRC) as new program to GNU coreutils, so this chokes the whole
script on gNewSense DeltaH (just as an example, there are many other
GNU/Linux distros in wider use that have only the traditional BSD
mktemp).  I'd suggest using short options here, for backward
compatibility.

| function init_delims()

Again, excessive indentation in the implementation.  There should be a
space between init_delims and the the parens which list the arguments
(whether present or not).

|               DELIMS=( "address@hidden" )

Does this assignment really require a space between the open/close
parens?  It smells like a bug if it does.

|                       echo "# ::File:: ${arg}" ; echo ""

Simply from aesthetic point of view this does not seem like a good thing
to me.  Why the double colons?

| function extract_and_diff()
...
|       <"${pofile}" gawk "${AWK_EXTRACT_SELECT}" records="${RECORDS}" \

This (and other usages of `gawk') are very problematic for systems where
gawk is not installed by default (e.g. Debian, gNewSense, etc. -- mawk
is the default there).  So the solution is:

     I. Check all your awk constructs against the common implementations
        available nowadays (all we care from GNUN POV is to work on all
        free systems -- all variants of GNU, all free *BSD systems,
        eventually OpenSolaris and the like).  We don't care about
        proprietary systems, which should narrow your search quite a
        bit.
    II. If you find out from I) that you use quite common and widely
        available awk constructs, there is no need to use `gawk' in the
        script, just use `awk', which would invoke the implementation
        which the site admin/user decided with no ill effects.
   III. If some awk implementations are lacking, we need to craft an
        autoconf test and use @AWK@ in gnun-view-fuzzy.in.  The script
        should not be built if a capable awk implementation is not
        found, and should substitute the AWK variable appropriately with
        the working awk implementation.  Don't worry about the
        implementation of this -- what's important is to identify the
        non-portable constructs; the rest is easy and I can do it
        myself.