bug#36009: [PATCH] Use lexical-binding in textmodes/page.el and add tests

bug#36009: [PATCH] Use lexical-binding in textmodes/page.el and add tests

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

I've written unit tests and added the lexical-binding header to
textmodes/page.el.  Please let me know if you have any comments.

Thanks,
Stefan Kangas

[-- Attachment #2: 0001-Use-lexical-binding-in-textmodes-page.el-and-add-tes.patch --]
[-- Type: text/x-patch, Size: 4464 bytes --]

From 87c2cb96d73090abe2761bd8044aa4aa5d84de90 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Tue, 28 May 2019 14:24:42 +0200
Subject: [PATCH] Use lexical-binding in textmodes/page.el and add tests

* lisp/textmodes/page.el: Use lexical-binding.
* test/lisp/textmodes/page-tests.el: New file.
---
 lisp/textmodes/page.el            |   2 +-
 test/lisp/textmodes/page-tests.el | 102 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+), 1 deletion(-)
 create mode 100644 test/lisp/textmodes/page-tests.el

diff --git a/lisp/textmodes/page.el b/lisp/textmodes/page.el
index 220ef2d7fd..768ca4482f 100644
--- a/lisp/textmodes/page.el
+++ b/lisp/textmodes/page.el
@@ -1,4 +1,4 @@
-;;; page.el --- page motion commands for Emacs
+;;; page.el --- page motion commands for Emacs  -*- lexical-binding: t -*-
 
 ;; Copyright (C) 1985, 2001-2019 Free Software Foundation, Inc.
 
diff --git a/test/lisp/textmodes/page-tests.el b/test/lisp/textmodes/page-tests.el
new file mode 100644
index 0000000000..5b8622084c
--- /dev/null
+++ b/test/lisp/textmodes/page-tests.el
@@ -0,0 +1,102 @@
+;;; page-tests.el --- Tests for page.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2019 Free Software Foundation, Inc.
+
+;; Author: Stefan Kangas <stefankangas@gmail.com>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'page)
+
+(defvar page-tests-page-1 "Page 1/3"
+  "Example text used to test page.el.")
+(defvar page-tests-page-2 "\nPage 2/3"
+  "Example text used to test page.el.")
+(defvar page-tests-page-3 "\nPage 3/3\nLast line"
+  "Example text used to test page.el.")
+(defvar page-tests-break "\n\f"
+  "Page break used to test page.el.")
+(defvar page-tests-example-text
+  (concat page-tests-page-1 page-tests-break
+          page-tests-page-2 page-tests-break
+          page-tests-page-3)
+  "Example text used to test page.el.")
+
+(ert-deftest page-tests-forward-page ()
+  (with-temp-buffer
+    (insert page-tests-example-text)
+    (goto-char (point-min))
+    (forward-page)
+    (should (looking-at page-tests-page-2))
+    (forward-page)
+    (should (looking-at page-tests-page-3))
+    (forward-page)
+    (should (equal (point) (point-max)))))
+
+(ert-deftest page-tests-backward-page ()
+  (with-temp-buffer
+    (insert page-tests-example-text)
+    (goto-char (point-max))
+    (backward-page)
+    (should (looking-at page-tests-page-3))
+    (backward-page)
+    (should (looking-at page-tests-page-2))
+    (backward-page)
+    (should (looking-at page-tests-page-1))
+    (should (equal (point) (point-min)))))
+
+(ert-deftest page-tests-mark-page ()
+  (with-temp-buffer
+    (insert page-tests-example-text)
+    (goto-char (point-min))
+    (mark-page)
+    (should (looking-at page-tests-page-1))
+    (should mark-active)
+    (should (equal (mark) 11))))
+
+(ert-deftest page-tests-narrow-to-page ()
+  (with-temp-buffer
+    (insert page-tests-example-text)
+    (goto-char (point-min))
+    (narrow-to-page)
+    (should (equal (buffer-string)
+                   (concat page-tests-page-1 "\n")))))
+
+(ert-deftest page-tests-count-lines-page ()
+  (with-temp-buffer
+    (insert page-tests-example-text)
+    (goto-char (point-min))
+    (should (equal (count-lines-page) "Page has 1 lines (0 + 1)"))
+    (goto-char (point-max))
+    (should (equal (count-lines-page) "Page has 3 lines (3 + 0)"))))
+
+(ert-deftest page-tests-what-page ()
+  (with-temp-buffer
+    (insert page-tests-example-text)
+    (goto-char (point-min))
+    (should (equal (what-page) "Page 1, line 1"))
+    (forward-page)
+    (should (equal (what-page) "Page 2, line 2"))
+    (forward-page)
+    (should (equal (what-page) "Page 3, line 4"))))
+
+(provide 'page-tests)
+;;; page-tests.el ends here
-- 
2.11.0

bug#36009: [PATCH] Use lexical-binding in textmodes/page.el and add tests

[Paul Eggert]

Stefan, the patch you proposed here:

https://debbugs.gnu.org/36009

conflicts with the more-recent patch installed by Simen here:

https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=f2a7acdde7207f92de53352f17dd7f506e15a851

My guess is that the Bug#36009 patch is now moot (or at least mootish); Simen is 
more qualified than I to judge.

bug#36009: [PATCH] Use lexical-binding in textmodes/page.el and add tests

[Simen Heggestøyl]

Oh, sorry Stefan, what a coincidence! I chose that file at random
yesterday. I should do a Debbugs search in advance next time.

Regarding the tests they look almost identical, except that I left out
tests for `count-lines-page' and `what-page', because I was unsure
whether they would be too dependent on the format of the user messages.

Maybe it would be good to factor out the meat of `count-lines-page'
and `what-page' into two internal functions returning just the raw
numbers, test those, and turn the interactive functions into
interfaces for them?

-- Simen

bug#36009: [PATCH] Use lexical-binding in textmodes/page.el and add tests

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

Simen Heggestøyl <simenheg@gmail.com> writes:
> Oh, sorry Stefan, what a coincidence! I chose that file at random
> yesterday. I should do a Debbugs search in advance next time.

No worries -- this fruit was clearly ripe for the taking. :)

> Regarding the tests they look almost identical, except that I left out
> tests for `count-lines-page' and `what-page', because I was unsure
> whether they would be too dependent on the format of the user messages.
>
> Maybe it would be good to factor out the meat of `count-lines-page'
> and `what-page' into two internal functions returning just the raw
> numbers, test those, and turn the interactive functions into
> interfaces for them?

Thanks for your input.

I did an experiment in your suggested direction, but it didn't come
out very successful.  In particular, I felt like I had to make this
simple and straightforward code ugly and convoluted.  I could attach
the final result of that experiment here, but I don't think it's very
helpful.

Based on this, I'm instead suggesting the attached simple patch for
installation.  It depends on the message string, but this should be
easy to update in the future if the format changes.

Thanks,
Stefan Kangas

[-- Attachment #2: 0001-Add-tests-for-count-lines-page-and-what-page.patch --]
[-- Type: text/x-patch, Size: 1461 bytes --]

From ff0fa006cae4b6220a5e765e25f3eef1f46edef0 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Thu, 13 Jun 2019 23:46:04 +0200
Subject: [PATCH] Add tests for count-lines-page and what-page

* test/lisp/textmodes/page-tests.el (page-tests-count-lines-page)
(page-tests-what-page): New test cases. (bug#36009)
---
 test/lisp/textmodes/page-tests.el | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/test/lisp/textmodes/page-tests.el b/test/lisp/textmodes/page-tests.el
index 0834d65433..8d2a036022 100644
--- a/test/lisp/textmodes/page-tests.el
+++ b/test/lisp/textmodes/page-tests.el
@@ -82,5 +82,24 @@ page-tests--region-string
     (narrow-to-page -1)
     (should (equal (buffer-string) "bar\n"))))
 
+(ert-deftest page-tests-count-lines-page ()
+  (with-temp-buffer
+    (insert "foo\n\f\nbar\n\f\nbaz")
+    (goto-char (point-min))
+    (should (equal (count-lines-page) "Page has 1 line (0 + 1)"))
+    (goto-char (point-max))
+    (count-lines-page)
+    (should (equal (count-lines-page) "Page has 2 lines (2 + 0)"))))
+
+(ert-deftest page-tests-what-page ()
+  (with-temp-buffer
+    (insert "foo\n\f\nbar\n\f\nbaz")
+    (goto-char (point-min))
+    (should (equal (what-page) "Page 1, line 1"))
+    (forward-page)
+    (should (equal (what-page) "Page 2, line 2"))
+    (forward-page)
+    (should (equal (what-page) "Page 3, line 4"))))
+
 (provide 'page-tests)
 ;;; page-tests.el ends here
-- 
2.11.0

bug#36009: [PATCH] Use lexical-binding in textmodes/page.el and add tests

[Simen Heggestøyl]

[-- Attachment #1.1: Type: text/plain, Size: 49 bytes --]

How about something along these lines?

-- Simen

[-- Attachment #1.2: Type: text/html, Size: 121 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Split-up-and-add-tests-for-two-page.el-functions.patch --]
[-- Type: text/x-patch, Size: 4774 bytes --]

From aaa78cb17dbfcfa8e7301d3e6734c5157a6da4ab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
Date: Sat, 22 Jun 2019 12:49:04 +0200
Subject: [PATCH] Split up and add tests for two page.el functions

* lisp/textmodes/page.el (page--count-lines-page): New function
extracted from `count-lines-page'.
(count-lines-page): Extract main logic into `page--count-lines-page'.
(page--what-page); New function extracted from `what-page'.
(what-page): Extract main logic into `page--what-page'.

* test/lisp/textmodes/page-tests.el (page-tests-count-lines-page)
(page-tests-what-page): New tests for `page--count-lines-page' and
`page--what-page'.
---
 lisp/textmodes/page.el            | 59 ++++++++++++++++++-------------
 test/lisp/textmodes/page-tests.el | 19 +++++++++-
 2 files changed, 52 insertions(+), 26 deletions(-)

diff --git a/lisp/textmodes/page.el b/lisp/textmodes/page.el
index 8921b697f3..a42fc6e053 100644
--- a/lisp/textmodes/page.el
+++ b/lisp/textmodes/page.el
@@ -125,41 +125,50 @@ narrow-to-page
 			(point)))))
 (put 'narrow-to-page 'disabled t)
 
-(defun count-lines-page ()
-  "Report number of lines on current page, and how many are before or after point."
-  (interactive)
+(defun page--count-lines-page ()
+  "Return a list of line counts on the current page.
+The list is on the form (TOTAL BEFORE AFTER), where TOTAL is the
+total number of lines on the current page, while BEFORE and AFTER
+are the number of lines on the current page before and after
+point, respectively."
   (save-excursion
-    (let ((opoint (point)) beg end
-	  total before after)
+    (let ((opoint (point)))
       (forward-page)
       (beginning-of-line)
-      (or (looking-at page-delimiter)
-	  (end-of-line))
-      (setq end (point))
-      (backward-page)
-      (setq beg (point))
-      (setq total (count-lines beg end)
-	    before (count-lines beg opoint)
-	    after (count-lines opoint end))
-      (message (ngettext "Page has %d line (%d + %d)"
-			 "Page has %d lines (%d + %d)" total)
-	       total before after))))
+      (unless (looking-at page-delimiter)
+        (end-of-line))
+      (let ((end (point)))
+        (backward-page)
+        (list (count-lines (point) end)
+              (count-lines (point) opoint)
+              (count-lines opoint end))))))
 
-(defun what-page ()
-  "Print page and line number of point."
+(defun count-lines-page ()
+  "Report number of lines on current page, and how many are before or after point."
   (interactive)
+  (pcase-let ((`(,total ,before ,after) (page--count-lines-page)))
+    (message (ngettext "Page has %d line (%d + %d)"
+                       "Page has %d lines (%d + %d)" total)
+             total before after)))
+
+(defun page--what-page ()
+  "Return a list of the page and line number of point."
   (save-restriction
     (widen)
     (save-excursion
       (let ((count 1)
-	    (opoint (point)))
-	(goto-char (point-min))
-	(while (re-search-forward page-delimiter opoint t)
-          (if (= (match-beginning 0) (match-end 0))
-              (forward-char 1))
-	  (setq count (1+ count)))
-	(message "Page %d, line %d" count (line-number-at-pos opoint))))))
+            (opoint (point)))
+        (goto-char (point-min))
+        (while (re-search-forward page-delimiter opoint t)
+          (when (= (match-beginning 0) (match-end 0))
+            (forward-char))
+          (setq count (1+ count)))
+        (list count (line-number-at-pos opoint))))))
 
+(defun what-page ()
+  "Print page and line number of point."
+  (interactive)
+  (apply #'message (cons "Page %d, line %d" (page--what-page))))
 
 \f
 ;;; Place `provide' at end of file.
diff --git a/test/lisp/textmodes/page-tests.el b/test/lisp/textmodes/page-tests.el
index 0834d65433..517f1d5a9e 100644
--- a/test/lisp/textmodes/page-tests.el
+++ b/test/lisp/textmodes/page-tests.el
@@ -82,5 +82,22 @@ page-tests-narrow-to-page
     (narrow-to-page -1)
     (should (equal (buffer-string) "bar\n"))))
 
-(provide 'page-tests)
+(ert-deftest page-tests-count-lines-page ()
+  (with-temp-buffer
+    (insert "foo\n\f\nbar\n\f\nbaz")
+    (goto-char (point-min))
+    (should (equal (page--count-lines-page) '(1 0 1)))
+    (goto-char (point-max))
+    (should (equal (page--count-lines-page) '(2 2 0)))))
+
+(ert-deftest page-tests-what-page ()
+  (with-temp-buffer
+    (insert "foo\n\f\nbar\n\f\nbaz")
+    (goto-char (point-min))
+    (should (equal (page--what-page) '(1 1)))
+    (forward-page)
+    (should (equal (page--what-page) '(2 2)))
+    (forward-page)
+    (should (equal (page--what-page) '(3 4)))))
+
 ;;; page-tests.el ends here
-- 
2.20.1

bug#36009: [PATCH] Use lexical-binding in textmodes/page.el and add tests

[Stefan Kangas]

Simen Heggestøyl <simenheg@gmail.com> writes:
> How about something along these lines?

Indeed, that's better than what I could come up with.
+1 from me.

Thanks,
Stefan Kangas

bug#36009: [PATCH] Use lexical-binding in textmodes/page.el and add tests

[Paul Eggert]

Stefan Kangas wrote:
> Simen Heggestøyl <simenheg@gmail.com> writes:
>> How about something along these lines?
> Indeed, that's better than what I could come up with.
> +1 from me.

Thanks. Simen, would you please install this?

bug#36009: [PATCH] Use lexical-binding in textmodes/page.el and add tests

[Simen Heggestøyl]

[-- Attachment #1: Type: text/plain, Size: 100 bytes --]

Paul Eggert <eggert@cs.ucla.edu> writes:

 > Simen, would you please install this?

Done!

-- Simen

[-- Attachment #2: Type: text/html, Size: 239 bytes --]