A cleaning-up patch for parse-time.el

A cleaning-up patch for parse-time.el

[Marcin Borkowski]

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

Hi Emacs Devs,

in my attempt to enhance Org-mode's date/time parsing, I found out that
I'll probably need to submit some modifications to `parse-time-string'.
While studying parse-time.el, I encountered some code whose purpose was
not obvious at the first glance, so I corrected it.  I attach a patch.
Is submitting such a patch a good idea?  (It does not introduce any new
features or bugfixes, just cleaning up: adding/expanding docstrings,
making one argument name better, adding a newline, and changing `not'
into `null'.)  How do I write a commit message for that?  (I went for
simplicity/terseness, since the diff speaks for itself.)

Thanks for your patience and best regards,

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Make-the-parse-time-tokenize-more-readable.patch --]
[-- Type: text/x-patch, Size: 1995 bytes --]

From fa2524bd41641d18bad51ca0a99986102d900568 Mon Sep 17 00:00:00 2001
From: Marcin Borkowski <mbork@mbork.pl>
Date: Sat, 19 Mar 2016 08:05:17 +0100
Subject: [PATCH] Make the `parse-time-tokenize' more readable

* lisp/calendar/parse-time.el (parse-time-string-chars)
(parse-time-tokenize): Make code more readable
---
 lisp/calendar/parse-time.el | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/lisp/calendar/parse-time.el b/lisp/calendar/parse-time.el
index 6ba26a4..33e936c 100644
--- a/lisp/calendar/parse-time.el
+++ b/lisp/calendar/parse-time.el
@@ -40,15 +40,18 @@
 (defvar parse-time-elt)
 (defvar parse-time-val)
 
-(defsubst parse-time-string-chars (char)
-  (cond ((<= ?a char ?z) ?a)
-        ((<= ?0 char ?9) ?0)
-        ((eq char ?+) 1)
-        ((eq char ?-) -1)
-        ((eq char ?:) ?d)))
+(defsubst parse-time-string-chars (character)
+  "Classify CHARACTER for `parse-time-tokenize'."
+  (cond ((<= ?a character ?z) ?a)
+        ((<= ?0 character ?9) ?0)
+        ((eq character ?+) 1)
+        ((eq character ?-) -1)
+        ((eq character ?:) ?d)))
 
 (defun parse-time-tokenize (string)
-  "Tokenize STRING into substrings."
+  "Tokenize STRING into substrings.
+Each substring is a run of \"valid\" characters, i.e., lowercase
+letters, digits, plus or minus signs or colons."
   (let ((start nil)
 	(end (length string))
 	(all-digits nil)
@@ -57,9 +60,10 @@ parse-time-tokenize
 	(c nil))
     (while (< index end)
       (while (and (< index end)		;Skip invalid characters.
-		  (not (setq c (parse-time-string-chars (aref string index)))))
+		  (null (setq c (parse-time-string-chars (aref string index)))))
 	(cl-incf index))
-      (setq start index all-digits (eq c ?0))
+      (setq start index
+            all-digits (eq c ?0))
       (while (and (< (cl-incf index) end)	;Scan valid characters.
 		  (setq c (parse-time-string-chars (aref string index))))
 	(setq all-digits (and all-digits (eq c ?0))))
-- 
2.4.3

Re: A cleaning-up patch for parse-time.el

[John Wiegley]

>>>>> Marcin Borkowski <mbork@mbork.pl> writes:

> in my attempt to enhance Org-mode's date/time parsing, I found out that
> I'll probably need to submit some modifications to `parse-time-string'.
> While studying parse-time.el, I encountered some code whose purpose was
> not obvious at the first glance, so I corrected it.  I attach a patch.
> Is submitting such a patch a good idea?  (It does not introduce any new
> features or bugfixes, just cleaning up: adding/expanding docstrings,
> making one argument name better, adding a newline, and changing `not'
> into `null'.)  How do I write a commit message for that?  (I went for
> simplicity/terseness, since the diff speaks for itself.)

Hi Marcin,

If you also include tests that show that the behavior is unchanged or merely
corrected from what we had before, then this sounds like something that would
be good for emacs-25. Otherwise, I'd suggest it be a patch against master,
pending review by others.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: A cleaning-up patch for parse-time.el

[Marcin Borkowski]

On 2016-03-19, at 08:41, John Wiegley <jwiegley@gmail.com> wrote:

>>>>>> Marcin Borkowski <mbork@mbork.pl> writes:
>
>> in my attempt to enhance Org-mode's date/time parsing, I found out that
>> I'll probably need to submit some modifications to `parse-time-string'.
>> While studying parse-time.el, I encountered some code whose purpose was
>> not obvious at the first glance, so I corrected it.  I attach a patch.
>> Is submitting such a patch a good idea?  (It does not introduce any new
>> features or bugfixes, just cleaning up: adding/expanding docstrings,
>> making one argument name better, adding a newline, and changing `not'
>> into `null'.)  How do I write a commit message for that?  (I went for
>> simplicity/terseness, since the diff speaks for itself.)
>
> Hi Marcin,
>
> If you also include tests that show that the behavior is unchanged or merely
> corrected from what we had before, then this sounds like something that would
> be good for emacs-25. Otherwise, I'd suggest it be a patch against master,
> pending review by others.

Hi John,

thanks for prompt response!

I think that the changes in this particular case are so cosmetic that
testing them would be overkill.  (Of course, I meant it to be applied to
master!)  OTOH, I expect to submit more complicated things in the
future, so I'll have to learn how to write tests for Emacs code, too.
Is there any documented policy on that or should I just look at the
tests already there?  (I can't see anything about testing in
admin/notes.)  Do I use ERT for testing?  Where do I put tests?

Best,

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University

Re: A cleaning-up patch for parse-time.el

[John Wiegley]

>>>>> Marcin Borkowski <mbork@mbork.pl> writes:

> I think that the changes in this particular case are so cosmetic that
> testing them would be overkill. (Of course, I meant it to be applied to
> master!) OTOH, I expect to submit more complicated things in the future, so
> I'll have to learn how to write tests for Emacs code, too. Is there any
> documented policy on that or should I just look at the tests already there?
> (I can't see anything about testing in admin/notes.) Do I use ERT for
> testing? Where do I put tests?

Please search for "test" in the CONTRIBUTE file within the repository you
checked out. It should lay out all the details you need.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: A cleaning-up patch for parse-time.el

[Michael Heerdegen]

John Wiegley <jwiegley@gmail.com> writes:

> Please search for "test" in the CONTRIBUTE file within the repository
> you checked out. It should lay out all the details you need.

Hmm, I really think that these changes are very obviously not changing
the behavior, everything is line breaks, a renamed variable, adding
docs, and one `not' replaced with `null'.

Everything looks fine.  I didn't read the code and have not verified
that the added doc makes sense, but I would expect they do.

So, my +1 for installing.


Michael.

Re: A cleaning-up patch for parse-time.el

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> So, my +1 for installing.

BTW, Marcin, do you have gotten commit rights meanwhile?


Michael.

Re: A cleaning-up patch for parse-time.el

[Lars Magne Ingebrigtsen]

Marcin Borkowski <mbork@mbork.pl> writes:

While the patch is obviously correct, parts of it isn't all that useful:

> * lisp/calendar/parse-time.el (parse-time-string-chars)
> (parse-time-tokenize): Make code more readable

[...]

> -(defsubst parse-time-string-chars (char)
> -  (cond ((<= ?a char ?z) ?a)
> -        ((<= ?0 char ?9) ?0)
> -        ((eq char ?+) 1)
> -        ((eq char ?-) -1)
> -        ((eq char ?:) ?d)))
> +(defsubst parse-time-string-chars (character)
> +  "Classify CHARACTER for `parse-time-tokenize'."
> +  (cond ((<= ?a character ?z) ?a)
> +        ((<= ?0 character ?9) ?0)
> +        ((eq character ?+) 1)
> +        ((eq character ?-) -1)
> +        ((eq character ?:) ?d)))

Using `char' instead of `character' is fine, I think.  (See `goto-char'
and a gazillion other usages in the Emacs source code.)

[...]

> -		  (not (setq c (parse-time-string-chars (aref string index)))))
> +		  (null (setq c (parse-time-string-chars (aref string index)))))

not vs null isn't very interesting, in my opinion.

> -      (setq start index all-digits (eq c ?0))
> +      (setq start index
> +            all-digits (eq c ?0))

This one makes sense, though (as does the comment fix).  I've applied
those bits to the trunk.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: A cleaning-up patch for parse-time.el

[Marcin Borkowski]

On 2016-03-25, at 16:24, Lars Magne Ingebrigtsen <larsi@gnus.org> wrote:

> Marcin Borkowski <mbork@mbork.pl> writes:
>
> While the patch is obviously correct, parts of it isn't all that useful:
> [...]
> not vs null isn't very interesting, in my opinion.

While I don't agree with that particular point (and no wonder,
I wouldn't change it otherwise;-)), I agree generally that the most
useful part of my patch was the docstring.

>> -      (setq start index all-digits (eq c ?0))
>> +      (setq start index
>> +            all-digits (eq c ?0))
>
> This one makes sense, though (as does the comment fix).  I've applied
> those bits to the trunk.

Thanks.

I have to admit that my goal was also to "test"the community: are such
patches welcome?  Now I understand that (1) correcting docstrings is
probably seen as useful and (2) general cleaning of the code sometimes
is and sometimes not, depending on how nitpicky it is.

I hope to provide more such patches in the future.  I'm aware that it's
not the thing Emacs wants or needs the most, but if I'm reading some
code anyway, why not correct it?

Best,

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University

Re: A cleaning-up patch for parse-time.el

[Lars Magne Ingebrigtsen]

Marcin Borkowski <mbork@mbork.pl> writes:

> I have to admit that my goal was also to "test"the community: are such
> patches welcome?  Now I understand that (1) correcting docstrings is
> probably seen as useful and (2) general cleaning of the code sometimes
> is and sometimes not, depending on how nitpicky it is.

Cleanliness is in the eyes of the observer.  :-)

> I hope to provide more such patches in the future.  I'm aware that it's
> not the thing Emacs wants or needs the most, but if I'm reading some
> code anyway, why not correct it?

Please do, but submit the patches with `M-x report-emacs-bug' so that
they don't get lost.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: A cleaning-up patch for parse-time.el

[John Wiegley]

>>>>> Lars Magne Ingebrigtsen <larsi@gnus.org> writes:

>> I hope to provide more such patches in the future. I'm aware that it's not
>> the thing Emacs wants or needs the most, but if I'm reading some code
>> anyway, why not correct it?

> Please do, but submit the patches with `M-x report-emacs-bug' so that they
> don't get lost.

Agreement on both points. :)  We welcome careful eyes over our code, just
beware that may run into stylistic points that change with the seasons.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2