joaotavora / sly

Sylvester the Cat's Common Lisp IDE
1.26k stars 142 forks source link

Port sly-tests.el timestamp formatting to future Emacs #481

Closed eggert closed 2 years ago

eggert commented 2 years ago

I installed the following into Savannah as a patch to Sly mode and then later realized that it needed to be installed upstream. Sorry, I don't do much ELPA hacking. Anyway, can you please arrange for it to be installed on GitHub too?

(I have sent a similar request to the Slime maintainers, since they have a similar problem.)

From 9300e466cb29257f6d3e0422384477038ad7011f Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 15 Dec 2021 09:36:12 -0800
Subject: [PATCH] Simplify message timestamp formatting

* lib/sly-tests.el (sly-wait-condition): Use format-time-string rather
than formatting by hand.  This avoids relying on Emacs timestamp
format, which is slated to change.
---
 lib/sly-tests.el | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/lib/sly-tests.el b/lib/sly-tests.el
index af381e9a56..44f50a5268 100644
--- a/lib/sly-tests.el
+++ b/lib/sly-tests.el
@@ -95,7 +95,7 @@ Exits Emacs when finished. The exit code is the number of failed tests."
                        (string-match "test/sly-\\(.*\\)\.elc?$" file-name))
                   (list 'contrib (intern (match-string 1 file-name)))
                 '(core)))))
-  
+
   (defmacro define-sly-ert-test (name &rest args)
     "Like `ert-deftest', but set tags automatically.
 Also don't error if `ert.el' is missing."
@@ -209,9 +209,8 @@ conditions (assertions)."
 (defun sly-wait-condition (name predicate timeout &optional cleanup)
   (let ((end (time-add (current-time) (seconds-to-time timeout))))
     (while (not (funcall predicate))
-      (let ((now (current-time)))
-        (sly-message "waiting for condition: %s [%s.%06d]" name
-                     (format-time-string "%H:%M:%S" now) (cl-third now)))
+      (sly-message "waiting for condition: %s [%s]" name
+                   (format-time-string "%H:%M:%S.%6N"))
       (cond ((time-less-p end (current-time))
              (unwind-protect
                  (error "Timeout waiting for condition: %S" name)
@@ -1314,7 +1313,7 @@ Reconnect afterwards."
       (with-current-buffer mrepl-buffer
         ;; FIXME: suboptimal: wait one second for the lisp
         ;; to reply.
-        (sit-for 1) 
+        (sit-for 1)
         (unless (and (string-match "^; +SLY" (buffer-string))
                      (string-match "CL-USER> *$" (buffer-string)))
           (die (format "MREPL prompt: %s" (buffer-string))))))))
@@ -1447,7 +1446,7 @@ Reconnect afterwards."
                                    (sly-test--eval-now "(.fn3.)"))
                              '("nil" "nil")))
           ;; Recompile now
-          ;; 
+          ;;
           (with-current-buffer xref-buffer
             (sly-recompile-all-xrefs)
             (sly-wait-condition "Compilation finished"
-- 
2.33.1
monnier commented 2 years ago

I installed the following into Savannah as a patch to Sly mode and then later realized that it needed to be installed upstream.

Indeed, pushing to nongnu.git is a bad idea since the auto-sync code can only handle fast-forwards, so any divergence between nongnu.git and upstream is a serious hassle.

We should try and make it harder to do it by accident.

Sorry, I don't do much ELPA hacking. Anyway, can you please arrange for it to be installed on GitHub too?

It needs to be git merged otherwise the fast-forward won't work. So, in case you don't like it (or parts of it), please first git merge it and then later undo the parts you don't like.

eggert commented 2 years ago

OK, to make it easy for João to do a git merge, I created a pull request.

joaotavora commented 2 years ago

Thanks, I made a whitespace tweak and pushed to master!

On Wed, Dec 15, 2021 at 8:20 PM Paul Eggert @.***> wrote:

OK, to make it easy for João to do a git merge, I created a pull request https://github.com/joaotavora/sly/pull/482.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/joaotavora/sly/issues/481#issuecomment-995183237, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC6PQYFUYZ7336H5XYCQ4LURD2ABANCNFSM5KEI5K3Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

-- João Távora

joaotavora commented 2 years ago

Ooops, I just saw this about the git merge. What Stefan? I What is the problem? Should I now revert and merge and revert unsolicited whitespace changes again afterwards. Seems awkward, but I can do that...

João

On Wed, Dec 15, 2021 at 9:27 PM João Távora @.***> wrote:

Thanks, I made a whitespace tweak and pushed to master!

On Wed, Dec 15, 2021 at 8:20 PM Paul Eggert @.***> wrote:

OK, to make it easy for João to do a git merge, I created a pull request https://github.com/joaotavora/sly/pull/482.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/joaotavora/sly/issues/481#issuecomment-995183237, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC6PQYFUYZ7336H5XYCQ4LURD2ABANCNFSM5KEI5K3Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

-- João Távora

-- João Távora

monnier commented 2 years ago

Ooops, I just saw this about the git merge. What Stefan? I What is the problem?

When we pull from upstream into nongnu.git we can only handle fast-forwards. And since Paul mistakenly pushed to nongnu.git we won't be able to pull from upstream until upstream's metadata says it has merged the actual commit id :-(

Should I now revert and merge and revert unsolicited whitespace changes again afterwards. Seems awkward, but I can do that...

Or do a git merge -s ours ...

monnier commented 2 years ago

Just a reminder that NonGNU ELPA is not following this repository any more currently because it thinks it has diverged:

% make fetch/sly       
emacs --batch -l admin/elpa-admin.el -f elpaa-batch-fetch-and-show "sly"
Fetching updates for sly...
Upstream of sly has DIVERGED!

  Local changes:
9300e466cb  eggert@cs.ucla..  Simplify message timestamp formatting

  Upstream changes:
19f4046dec  eggert@cs.ucla..  Simplify message timestamp formatting

%

So something like git merge -s ours 9300e466cb would be very appreciated.

joaotavora commented 2 years ago

Hi Stefan,

I just noticed you should have commit rights to Sly's GitHub upstream. So you should be able to do this merge yourself. I'm quite extremely busy at the moment and fear I'll mess it up somehow.

Thanks in advance, João

On Tue, Dec 21, 2021, 02:09 monnier @.***> wrote:

Just a reminder that NonGNU ELPA is not following this repository any more currently because it thinks it has diverged:

% make fetch/sly emacs --batch -l admin/elpa-admin.el -f elpaa-batch-fetch-and-show "sly" Fetching updates for sly... Upstream of sly has DIVERGED!

Local changes: 9300e466cb @.*** Simplify message timestamp formatting

Upstream changes: 19f4046dec @.*** Simplify message timestamp formatting

%

So something like git merge -s ours 9300e466cb would be very appreciated.

— Reply to this email directly, view it on GitHub https://github.com/joaotavora/sly/issues/481#issuecomment-998410189, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC6PQ7ACVVPBBRHOCXUHFTUR7OXBANCNFSM5KEI5K3Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>

joaotavora commented 2 years ago

Well, I just did the merge like @monnier suggested. I hope I didn't mess up anything :-). Tentatively closing this, but feel free to reopen if there is still something to do.

monnier commented 2 years ago

Hi Stefan, I just noticed you should have commit rights to Sly's GitHub upstream. So you should be able to do this merge yourself.

Duh! Can't believe I forgot that. Fixed, thanks.