bug#28637: [PATCH] Display commit in package description, if available

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#28637: [PATCH] Display commit in package description, if available

David Glasser-4
MELPA includes a :commit field in its
packages (https://github.com/melpa/package-build/pull/6). You can use
this to tell if MELPA has processed a recently-merged change.  This
commit adds that metadata to the package description buffer.

* lisp/emacs-lisp/package.el: Display commit in package description
---
 lisp/emacs-lisp/package.el | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 8b101c1323..dd05c70dc8 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2260,6 +2260,7 @@ Otherwise no newline is inserted."
          (archive (if desc (package-desc-archive desc)))
          (extras (and desc (package-desc-extras desc)))
          (homepage (cdr (assoc :url extras)))
+         (commit (cdr (assoc :commit extras)))
          (keywords (if desc (package-desc--keywords desc)))
          (built-in (eq pkg-dir 'builtin))
          (installable (and archive (not built-in)))
@@ -2332,6 +2333,8 @@ Otherwise no newline is inserted."
     (and version
          (package--print-help-section "Version"
            (package-version-join version)))
+    (when commit
+      (package--print-help-section "Commit" commit))
     (when desc
       (package--print-help-section "Summary"
         (package-desc-summary desc)))
--
2.14.1



Reply | Threaded
Open this post in threaded view
|

bug#28637: Display commit in package description, if available

David Glasser-4
Can I confirm that this went through successfully? Should I be
bringing up this patch on a different email list?



Reply | Threaded
Open this post in threaded view
|

bug#28637: Display commit in package description, if available

Eli Zaretskii
> From: David Glasser <[hidden email]>
> Date: Tue, 10 Oct 2017 12:35:58 -0700
>
> Can I confirm that this went through successfully?

AFAICT, no one has reviewed this yet.

> Should I be bringing up this patch on a different email list?

This is the right place, just keep pinging until the patch is
reviewed.

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#28637: Display commit in package description, if available

David Glasser-4
OK, sure!

I think this is a pretty minimal-impact change which makes the
experience of using melpa a lot better. I frequently send PRs to
packages that I fetch via melpa, and once I get the "merged"
notification, I want to be able to tell whether or not my change is
available in the melpa build.



Reply | Threaded
Open this post in threaded view
|

bug#28637: [PATCH] Display commit in package description, if available

Noam Postavsky-2
In reply to this post by David Glasser-4
David Glasser <[hidden email]> writes:

> MELPA includes a :commit field in its
> packages (https://github.com/melpa/package-build/pull/6). You can use
> this to tell if MELPA has processed a recently-merged change.  This
> commit adds that metadata to the package description buffer.

Code looks fine, but could you post a "before and after" picture please?
I'm wondering if it might make sense to abbreviate the hash.

> * lisp/emacs-lisp/package.el: Display commit in package description

I think you should have a period at the end of sentence though.



Reply | Threaded
Open this post in threaded view
|

bug#28637: [PATCH] Display commit in package description, if available

David Glasser-4
On Tue, Oct 10, 2017 at 3:40 PM, Noam Postavsky
<[hidden email]> wrote:
> David Glasser <[hidden email]> writes:
>
>> MELPA includes a :commit field in its
>> packages (https://github.com/melpa/package-build/pull/6). You can use
>> this to tell if MELPA has processed a recently-merged change.  This
>> commit adds that metadata to the package description buffer.
>
> Code looks fine, but could you post a "before and after" picture please?
> I'm wondering if it might make sense to abbreviate the hash.

Sure! Here's a randomly chosen melpa package's *Help* page:

```
archive-rpm is a new package.

     Status: New from melpa -- Install
    Archive: melpa
    Version: 20171005.1548
     Commit: 830158cfb3b43c85cfcb4bd5b92d4457d015c80a
    Summary: RPM and CPIO support for archive-mode
   Requires: emacs-24.4

This module adds support for RPM archives to archive-mode.

RPM files consist of metadata plus a compressed CPIO archive, so
this module relies on `archive-cpio'.
```

This is the "after" picture; the "before" picture lacks the Commit line.

Abbreviating the hash might be nice, but it seems like maybe that should be
the job of the code that creates the metadata (ie in melpa's package builder)
rather than the code that displays it.


>> * lisp/emacs-lisp/package.el: Display commit in package description
>
> I think you should have a period at the end of sentence though.

OK, a new version of the patch is below (is this the right way to send
a new version of the patch? I am not used to mail-based git
workflows):

From a4ebfa2ed35a620e4754399da8181caba13a1eb9 Mon Sep 17 00:00:00 2001
From: David Glasser <[hidden email]>
Date: Thu, 28 Sep 2017 14:00:04 -0700
Subject: [PATCH] Display commit in package description, if available

MELPA includes a :commit field in its
packages (https://github.com/melpa/package-build/pull/6). You can use
this to tell if MELPA has processed a recently-merged change.  This
commit adds that metadata to the package description buffer.

* lisp/emacs-lisp/package.el: Display commit in package description.
---
 lisp/emacs-lisp/package.el | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 8b101c1323..dd05c70dc8 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -2260,6 +2260,7 @@ Otherwise no newline is inserted."
          (archive (if desc (package-desc-archive desc)))
          (extras (and desc (package-desc-extras desc)))
          (homepage (cdr (assoc :url extras)))
+         (commit (cdr (assoc :commit extras)))
          (keywords (if desc (package-desc--keywords desc)))
          (built-in (eq pkg-dir 'builtin))
          (installable (and archive (not built-in)))
@@ -2332,6 +2333,8 @@ Otherwise no newline is inserted."
     (and version
          (package--print-help-section "Version"
            (package-version-join version)))
+    (when commit
+      (package--print-help-section "Commit" commit))
     (when desc
       (package--print-help-section "Summary"
         (package-desc-summary desc)))



Reply | Threaded
Open this post in threaded view
|

bug#28637: [PATCH] Display commit in package description, if available

Noam Postavsky-2
David Glasser <[hidden email]> writes:

> Abbreviating the hash might be nice, but it seems like maybe that should be
> the job of the code that creates the metadata (ie in melpa's package builder)
> rather than the code that displays it.

Hmm, I'm not so sure about that, but looking at the result now I think
there's no need to abbreviate it anyway.

I think this should be okay to go to emacs-26; it's not strictly
speaking a bug fix, but it's obviously safe and this info is often
useful for package maintainers fielding bug reports, so it would be good
to have it more widely available.  Eli?

> OK, a new version of the patch is below (is this the right way to send
> a new version of the patch? I am not used to mail-based git
> workflows):

Yup, that's fine.  You can also use the -v option to git-format-patch in
order to make it more obvious that the patch is new.  For small stuff
like this, it's not really important though.

I suppose you haven't assigned copyright for Emacs?  No problem if not,
the patch is small enough to go in anyway, we just have to mark it.



Reply | Threaded
Open this post in threaded view
|

bug#28637: [PATCH] Display commit in package description, if available

David Glasser-4
On Oct 10, 2017 6:16 PM, "Noam Postavsky" <[hidden email]> wrote:
I suppose you haven't assigned copyright for Emacs?  No problem if not,
the patch is small enough to go in anyway, we just have to mark it.

I don't believe I have. 

--dave
Reply | Threaded
Open this post in threaded view
|

bug#28637: [PATCH] Display commit in package description, if available

Eli Zaretskii
In reply to this post by Noam Postavsky-2
> From: Noam Postavsky <[hidden email]>
> Date: Tue, 10 Oct 2017 21:16:36 -0400
> Cc: [hidden email]
>
> I think this should be okay to go to emacs-26; it's not strictly
> speaking a bug fix, but it's obviously safe and this info is often
> useful for package maintainers fielding bug reports, so it would be good
> to have it more widely available.  Eli?

Are we sure no feature parses this output, and might be confused by
the extra line?



Reply | Threaded
Open this post in threaded view
|

bug#28637: [PATCH] Display commit in package description, if available

Noam Postavsky-2
Eli Zaretskii <[hidden email]> writes:

>> From: Noam Postavsky <[hidden email]>
>> Date: Tue, 10 Oct 2017 21:16:36 -0400
>> Cc: [hidden email]
>>
>> I think this should be okay to go to emacs-26; it's not strictly
>> speaking a bug fix, but it's obviously safe and this info is often
>> useful for package maintainers fielding bug reports, so it would be good
>> to have it more widely available.  Eli?
>
> Are we sure no feature parses this output, and might be confused by
> the extra line?

As far as I can tell, it's only used for the *Help* buffer, and nothing
parses that.



Reply | Threaded
Open this post in threaded view
|

bug#28637: [PATCH] Display commit in package description, if available

Eli Zaretskii
> From: Noam Postavsky <[hidden email]>
> Cc: [hidden email],  [hidden email]
> Date: Wed, 11 Oct 2017 08:32:43 -0400
>
> Eli Zaretskii <[hidden email]> writes:
>
> >> From: Noam Postavsky <[hidden email]>
> >> Date: Tue, 10 Oct 2017 21:16:36 -0400
> >> Cc: [hidden email]
> >>
> >> I think this should be okay to go to emacs-26; it's not strictly
> >> speaking a bug fix, but it's obviously safe and this info is often
> >> useful for package maintainers fielding bug reports, so it would be good
> >> to have it more widely available.  Eli?
> >
> > Are we sure no feature parses this output, and might be confused by
> > the extra line?
>
> As far as I can tell, it's only used for the *Help* buffer, and nothing
> parses that.

Then it could be okay for emacs-26, if its addition is deemed
important enough.