Re: [PATCH] elpa/packages/sokoban/sokoban.el

[Stefan Monnier <monnier@iro.umontreal.ca>]

> +  (with-temp-buffer
> +    (insert-file-contents sokoban-level-file)
> +    (goto-char (point-min))
> +    (if (looking-at "<\\?xml version=")
> +        (let ((n 0) (tree (xml-parse-region)))
> +          (erase-buffer)
> +          (dolist (SokobanLevels tree)
> +            (dolist (LevelCollection (xml-get-children SokobanLevels 'LevelCollection))
> +              (dolist (Level (xml-get-children LevelCollection 'Level))
> +                (incf n)
> +                (insert (format ";LEVEL %d\n" n))
> +                (dolist (L (xml-get-children Level 'L))
> +                  (insert (car (xml-node-children L)))
> +                  (insert "\n")))))))

Why not put this code in its own function?

> @@ -884,8 +896,8 @@ sokoban-mode keybindings:
>            '("Sokoban Commands"
>              ["Restart this level" sokoban-restart-level]
>              ["Start new game" sokoban-start-game]
> -            ["Go to specific level" sokoban-goto-level])))
> -
> +            ["Go to specific level" sokoban-goto-level]
> +            ["Fit frame to buffer" fit-frame-to-buffer])))

I doubt the current sokoban.el works in XEmacs, so I'd get rid of the
XEmacs-specific code.

Other than that, looks very good,


        Stefan