From: tiantian <typ22@foxmail.com>
To: Julien Lepiller <julien@lepiller.eu>
Cc: 57496@debbugs.gnu.org
Subject: [bug#57496] [PATCH v3 3/3] gnu: bootloader: Add a friendly error message of menu-entry.
Date: Mon, 05 Sep 2022 00:15:12 +0800 [thread overview]
Message-ID: <tencent_5C1AB50746A10D46B978178C728B0003EC07@qq.com> (raw)
Message-ID: <7x7d2jt5z0.fsf@foxmail.com> (raw)
In-Reply-To: <20220904173755.5be9efa0@sybil.lepiller.eu>
Julien Lepiller <julien@lepiller.eu> writes:
> Hi tiantian,
>
> I think the first two patches are good now, so let me focus on this one
> :)
>
> Le Sun, 4 Sep 2022 22:04:31 +0800,
> typ22@foxmail.com a écrit :
>
>> From: tiantian <typ22@foxmail.com>
>>
>> + (define (set-field? field)
>> + "If the field is the default value, return #f."
>> + (and field ; not default value #f
>> + (not (null? field)))) ; not default value '()
>
> I don't think this set-field? is necessary. In the following, I don't
> think you need the null? case at all because I think all the lists may
> be empty without triggering an error. You don't necessarily want to
> load modules or have arguments on the linux command line.
>
> In any case, it should be called field-set? instead :)
>
My English is not good. To be honest, I tried set-value?, value-set?,
default-value?, not-default-value?, field-set? and set-field?. Finally,
I select the 'set-field?'. But it seems that I didn't choose well.
But it doesn't matter. The procedure is no longer needed.
>> (match entry
>> + ;; Modify the pattern to achieve more strict matching and prevent
>> + ;; wrong matching, which ensures the output of error information
>> + ;; when menu-entry is wrong.
>> +
>> + ;; The linux-arguments is optional and the test code for
>> boot-parameters
>> + ;; does not set it, so don't check it here.
>> (($ <menu-entry> label device mount-point
>> - (? identity linux) linux-arguments initrd
>> + (? set-field? linux)
>> + linux-arguments
>> + (? set-field? initrd)
>
> The could still be identity
>
>> #f () () #f)
>> `(menu-entry (version 0)
>> (label ,label)
>> @@ -131,8 +162,10 @@ (define (menu-entry->sexp entry)
>> (linux-arguments ,linux-arguments)
>> (initrd ,initrd)))
>> (($ <menu-entry> label device mount-point #f () #f
>> - (? identity multiboot-kernel)
>> multiboot-arguments
>> - multiboot-modules #f)
>> + (? set-field? multiboot-kernel)
>> + (? set-field? multiboot-arguments)
>> + (? set-field? multiboot-modules)
>
> Some users might want to not use any modules or arguments I think, so
> these fields should not be mandatory. For multiboot-kernel, identity is
> enough.
>
>> + #f)
>> `(menu-entry (version 0)
>> (label ,label)
>> (device ,(device->sexp device))
>> @@ -141,12 +174,13 @@ (define (menu-entry->sexp entry)
>> (multiboot-arguments ,multiboot-arguments)
>> (multiboot-modules ,multiboot-modules)))
>> (($ <menu-entry> label device mount-point #f () #f #f () ()
>> - (? identity chain-loader))
>> + (? set-field? chain-loader))
>
> Same here, identity is fine.
>
I don't know multiboot very well, so I don't know if multiboot-arguments
and multiboot-modules are necessary. Then I check them. It turns out that
they are not necessary. I will not check them.
I will change to use identify. Honestly, It's much easier for me to
use only `identify'. :)
>> `(menu-entry (version 0)
>> (label ,label)
>> (device ,(device->sexp device))
>> (device-mount-point ,mount-point)
>> - (chain-loader ,chain-loader)))))
>> + (chain-loader ,chain-loader)))
>> + (else (report-menu-entry-error entry))))
>
> Since this is a match, it is more common to use:
>
> (_ (report-menu-entry-error entry))
>
Thank you for reminding me. I will correct it.
> Also, it feels weird to patch the code you modified in a previous patch
> of the same series. If you're not happy with the code you wrote in a
> previous patch, you need to change it in the previous patch, not in a
> new one :)
>
As I understood earlier, these changes about matching are related to the
error reporting information, so I put these modifications in this
submission. My knowledge of contribution is still too little.
I will pay attention to it later. Thank you for reminding me.
Thanks,
tiantian
prev parent reply other threads:[~2022-09-04 17:18 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <849b9ddea2a18dc4b2fd05450f0c90e3e5a05421.1662298270.git.typ22@foxmail.com>
2022-09-04 14:04 ` [bug#57496] [PATCH v3 2/3] gnu: bootloader: grub: Add support for chain-loader typ22
2022-09-04 14:04 ` [bug#57496] [PATCH v3 3/3] gnu: bootloader: Add a friendly error message of menu-entry typ22
2022-09-04 15:37 ` Julien Lepiller
[not found] ` <7x7d2jt5z0.fsf@foxmail.com>
2022-09-04 16:15 ` tiantian [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://guix.gnu.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=tencent_5C1AB50746A10D46B978178C728B0003EC07@qq.com \
--to=typ22@foxmail.com \
--cc=57496@debbugs.gnu.org \
--cc=julien@lepiller.eu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/guix.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).