[darcs-users] darcs patch: Resolve issue1373: make --token-chars [^ \t\n] work as...

[Eric Kow]

Hi Trent,

Applied, with some comments about future patches and perhaps a
request for future tidying.

Thanks!

On Sat, Mar 28, 2009 at 21:43:50 +1100, Trent W.Buck wrote:
> Addresses issues Eric raised in previous submission.
> 
> Sat Mar 28 21:41:48 EST 2009  Trent W. Buck <trentbuck at gmail.com>
>   * Resolve issue1373: make --token-chars [^ \t\n] work as advertised.

Resolve issue1373: make --token-chars [^ \t\n] work as advertised.
------------------------------------------------------------------
> +-- | Given a set of characters and a string, returns true iff the
> +-- string contains only characters from the set.  A set beginning with
> +-- a caret (@^@) is treated as a complementary set.

I'm glad to see increased comment-coverage, but I think these should
have gone in a separate patch since they aren't /really/ related to the
change in question.  It's a bit subtle, I realise. I am trying to be
consistent, but may accidentally contradict myself from time to time.
For example, a patch that changes a function in a way that obsoletes the
comment should probably include the comment tweak too.  It's tricky to
get all this written down in policy, but I think we share the same basic
principles of working to make sure that patches are as easy to
understand and as orthogonal as is reasonable.  Figuring out what that
means in each new case is the hard part :-)

> +-- | This function checks for @--token-chars@ on the command-line.  If
> +-- found, it validates the argument and returns it, without the
> +-- surrounding square brackets.  Otherwise, it returns either
> +-- 'default_toks' or 'filename_toks' as explained in 'replace_help'.
>  choose_toks :: [DarcsFlag] -> String -> String -> IO String
>  choose_toks (Toks t:_) a b
> hunk ./src/Darcs/Commands/Replace.lhs 207
> -    | any isSpace t = fail $ bad_token_spec $ "Space is not allowed in the spec"
> -    | length t <= 2 = fail $ bad_token_spec $
> -                        "It must contain more than 2 characters, because " ++
> -                        "it should be enclosed in square brackets"
> -    | head t /= '[' || last t /= ']' = fail $ bad_token_spec $
> -                        "It should be enclosed in square brackets"
> -    | not (is_tok tok a) = fail $ bad_token_spec $ not_a_token a
> -    | not (is_tok tok b) = fail $ bad_token_spec $ not_a_token b
> +    | length t <= 2 =
> +        bad_token_spec $ "It must contain more than 2 characters, because " ++
> +                         "it should be enclosed in square brackets"
> +    | head t /= '[' || last t /= ']' =
> +        bad_token_spec "It should be enclosed in square brackets"
> +    | '^' == head tok && length tok == 1 =

At first I was going to complain about not accounting for tok being
null, but then I saw that this was trapped by length t <= 2.  Fine,
but I think we can use Haskell's pattern matching facilities for more
robust code (although potentially less readable).

I wonder if we could do something like

  | otherwise = case init (tail t) of
                 []       ->
                 ('^':[]) -> ...
                 ('^':cs) ->
                 tok | is_tok tok a -> ...
                     | is_tok tok b -> ...
                     | otherwise    -> ...

If that doesn't work, then maybe an embedded case statement using
the case () of _ | guard -> syntax.

The tradeoff is that we would lose the nice property of everything being
one nice cascade of guards...

> +        bad_token_spec "Must be at least one character in the complementary set"
> +    | '^' /= head tok && any isSpace t =
> +        bad_token_spec "Space is not allowed in the spec"
> +    | not (is_tok tok a) = bad_token_spec $ not_a_token a
> +    | not (is_tok tok b) = bad_token_spec $ not_a_token b

I would have saved the bad_token_spec refactor for another patch within
this bundle.  It makes it clearer what has actually changed.

> addfile ./tests/issue1373_replace_token_chars.sh
> hunk ./tests/issue1373_replace_token_chars.sh 1

> +rm -rf temp                     # Another script may have left a mess.
> +darcs init --repodir temp
> +replace () { darcs replace --repodir temp --token-chars "$1" x y; }

> +not replace $'[\t]'
> +not replace $'[\n]'
> +not replace $'[\r]'
> +not replace $'[\v]'

> +## useless for us pro-i10n types.
> +replace $'[^ \t\n\r\v]'

Great, thanks.  I wonder if we should also include tests (if we don't
already have them, to check for the effects of the replace on the file)

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20090328/ce7c1be5/attachment.pgp>