Subject: Re: [bug#74174] [PATCH v2] gnu: Add emacs-cargo-el.

From: Hilton Chain <[email-masked]>
Date: Thu, 07 Nov 2024 09:25:10 +0800

Hi Javier,

On Tue, 05 Nov 2024 12:54:49 +0800,
Javier Olaechea wrote:
>
> [1  <multipart/alternative (7bit)>]
> [1.1  <text/plain; UTF-8 (quoted-printable)>]
> [1.2  <text/html; UTF-8 (quoted-printable)>]
> Guix QA review form submission:
>
> > - sentences in description should be followed by two spaces
>
> Updated the description accordingly
>
> > - check propagated-inputs, emacs-rust-mode instead of
> > emacs-markdown-mode ?
>
> No, the package doesn't depend on rust-mode at all. It shells out the cargo and rustc instead. The require
> calls in the elisp files are

We generally patch these references to paths within /gnu/store.

> (require 'xref)
> (require 'compile)
> (require 'button)
> (require 'markdown-mode)
> (require 'tramp)
>
> Of those, only emacs-markdown-mode is not a built-in package. Although we could add xref and tramp as inputs.
> They are also distributed as separate packages in GNU Elpa. This means they have a separate release cadence
> from emacs. What do you think?
>
> > - first revision should be 0 instead of 1 ?
>
> Thanks, it is true 'hackers count from 0'. ^_^. Updated.

There's no strict requirement on revisions :)
Just make sure (version + revision) is increasing when updating package source.

> Cheers
> --
> "I object to doing things that computers can do." ― Olin Shivers
> [2 v2-0001-gnu-Add-emacs-cargo-el.patch <text/x-patch; US-ASCII (base64)>]
> From 95bb683ce62fd38c9bbbc4efa4f3499d0cc4f647 Mon Sep 17 00:00:00 2001
> Message-ID: <[email-masked]>
> From: Javier Olaechea <[email-masked]>
> Date: Sat, 2 Nov 2024 00:36:57 -0500
> Subject: [PATCH v2] gnu: Add emacs-cargo-el

Don't forget to end the subject with ‘.’

> * gnu/packages/emacs-xyz.scm (emacs-cargo-el): New variable.
>
> Change-Id: I73a99eeb818fb1c7ab87cc15c5953beba818cb94
> ---
>  gnu/packages/emacs-xyz.scm | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/gnu/packages/emacs-xyz.scm b/gnu/packages/emacs-xyz.scm
> index 59c804066a..fc8a5839b4 100644
> --- a/gnu/packages/emacs-xyz.scm
> +++ b/gnu/packages/emacs-xyz.scm
> @@ -29667,6 +29667,29 @@ (define-public emacs-rustic
>      (license (list license:expat
>                     license:asl2.0))))
>
> +(define-public emacs-cargo-el
> +  (let ((commit "7f8466063381eed05d4e222ce822b1dd44e3bf17")
> +        (revision "0"))
> +    (package
> +      (name "emacs-cargo-el")
> +      (version "0.4.0")

The version 0.4.0 doesn't match upstream commit 7f8466063381, so we shouldn't
use it here.  Please see package guidelines on version numbers[1] for details.

> +      (source
> +       (origin
> +         (method git-fetch)
> +         (uri (git-reference
> +               (url "https://github.com/kwrooijen/cargo.el")
> +               (commit commit)))

Don't forget to add a file name to this origin, you can use ‘guix lint’ to
identify such issue.

> +         (sha256
> +          (base32 "1hvxdmyppvx04jyn07dnynlgbwyasv22k8dd4qa68mrj8i9mz484"))))
> +      (build-system emacs-build-system)
> +      (propagated-inputs (list emacs-markdown-mode))
> +      (home-page "https://github.com/kwrooijen/cargo.el")
> +      (synopsis "Emacs Minor Mode for Cargo, Rust's Package Manager")

Uppercase only when it's meaningful.

> +      (description
> +       "Cargo mode for Emacs.  This package gives you a set of key combinations to
> +perform Cargo tasks within your Rust projects.")

Use full sentences for description, "Cargo mode for Emacs" should be reworded.
Guidelines for synopses and descriptions are available in [2].

> +      (license license:gpl3+))))
> +
>  (define-public emacs-ztree
>    ;; Upstream provides no tag, but the commit below matches latest release.
>    (let ((commit "c9ad9136d52ca5a81475693864e255d29448f43f"))
>
> base-commit: 33665c52c4670bc3b4d337c89ac9cc6c4c69b26f
> --
> 2.46.0

I have adjusted your patch based on above comments and applied it as
673b924ac1e30a5d498e28859af365cf2bb4a508, thanks!

Also thanks for Cayetano's previous review!

---
[1]: https://guix.gnu.org/manual/devel/en/html_node/Version-Numbers.html
[2]: https://guix.gnu.org/manual/devel/en/html_node/Synopses-and-Descriptions.html