[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] elpa/packages/sokoban/sokoban.el
From: |
Stefan Monnier |
Subject: |
Re: [PATCH] elpa/packages/sokoban/sokoban.el |
Date: |
Tue, 25 Jul 2017 10:24:30 -0400 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.0.50 (gnu/linux) |
> + (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