| Message ID | cover.1716372146.git.andrew@trop.in |
|---|---|
| Headers | show |
| Series | services: home: Use pairs instead of lists. | expand |
Andrew Tropin <andrew@trop.in> writes: > After rewriting from car/cdr to match-lambda in v2 of this patch: > https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard@freakingpenguin.com/ > > the format changed from pairs to lists, I didn't noticed this nuance > during review because the documentation still says that service should > be configured and extended with pairs. Also, pairs are more > apropriate data type here. And this match-lambda rewrite will break > downstream RDE user's setups after migrating to upstreamed version of > service. > > That's why I propose to go back to pairs. > I'm not opposed to going back to cons cell pairs. I didn't put too much thought in a "list of two elements" vs. "cons cell" besides the match statement being easier to handle with a list. Would this patch have unintended side effects? I thought the . in match conditions had a different behavior. --8<---------------cut here---------------start------------->8--- scheme@(guile-user)> (match '(1 2 3 4) ((a . b) b)) $5 = (2 3 4) scheme@(guile-user)> (match '(1 2) ((a . b) b)) $6 = (2) scheme@(guile-user)> (match '(1 . 2) ((a . b) b)) $7 = 2 --8<---------------cut here---------------end--------------->8--- So changing to this would allow for a home-service entry like the following to match: --8<---------------cut here---------------start------------->8--- (simple-service 'my-extra-home home-service-type `(("bob" "bill" "bertha" "billyanne" ,my-extra-home ,my-home-2))) --8<---------------cut here---------------end--------------->8--- From my testing, this /will/ error (yay), but not as soon as the match would with the current code. Instead of being caught before being passed to the daemon, it seems to be caught while lowering the invalid file-append object. Personally I would prefer to catch as many errors as possible before beginning to pass code off to the daemon where possible. Generally speaking it feels like pre-daemon errors are easier to mentally parse. In fairness, the current code also isn't trying particularly hard to check that "user" is a string and "he" is a home-environment or printing a fancy error message. Perhaps this would work? --8<---------------cut here---------------start------------->8--- (define-module (gnu services guix) ... #:use-module (gnu home)) ... (define (guix-home-shepherd-service config) (map (match-lambda (((? string? user) . (? home-environment? he)) (shepherd-service ... )) (e (error "Invalid value for guix-home-shepherd-service: " e))) config)) --8<---------------cut here---------------end--------------->8--- Maybe I'm just being silly. 🙂
Andrew Tropin via Guix-patches via <guix-patches@gnu.org> writes: > After rewriting from car/cdr to match-lambda in v2 of this patch: > https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard@freakingpenguin.com/ > > the format changed from pairs to lists, I didn't noticed this nuance > during review because the documentation still says that service should > be configured and extended with pairs. Also, pairs are more > apropriate data type here. And this match-lambda rewrite will break > downstream RDE user's setups after migrating to upstreamed version of > service. > > That's why I propose to go back to pairs. Maybe we can support pairs and list of length two at same time? > > Andrew Tropin (1): > services: home: Use pairs instead of lists. > > doc/guix.texi | 4 ++-- > gnu/services/guix.scm | 2 +- > gnu/tests/guix.scm | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > > base-commit: b06a460bf5236a9d52f936f2023451051b3e622a
On 2024-05-23 11:38, Zheng Junjie wrote: > Andrew Tropin via Guix-patches via <guix-patches@gnu.org> writes: > >> After rewriting from car/cdr to match-lambda in v2 of this patch: >> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard@freakingpenguin.com/ >> >> the format changed from pairs to lists, I didn't noticed this nuance >> during review because the documentation still says that service should >> be configured and extended with pairs. Also, pairs are more >> apropriate data type here. And this match-lambda rewrite will break >> downstream RDE user's setups after migrating to upstreamed version of >> service. >> >> That's why I propose to go back to pairs. > > Maybe we can support pairs and list of length two at same time? Thank you for the idea, however I think ambiguity is a bad practice, from my early experience with guix it's more confusing rather than helpful. I still don't know why profile-service-type accepts list of lists rather than alist (list of pairs). > >> >> Andrew Tropin (1): >> services: home: Use pairs instead of lists. >> >> doc/guix.texi | 4 ++-- >> gnu/services/guix.scm | 2 +- >> gnu/tests/guix.scm | 2 +- >> 3 files changed, 4 insertions(+), 4 deletions(-) >> >> >> base-commit: b06a460bf5236a9d52f936f2023451051b3e622a
On 2024-05-22 17:33, Richard Sent wrote: > Andrew Tropin <andrew@trop.in> writes: > >> After rewriting from car/cdr to match-lambda in v2 of this patch: >> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard@freakingpenguin.com/ >> >> the format changed from pairs to lists, I didn't noticed this nuance >> during review because the documentation still says that service should >> be configured and extended with pairs. Also, pairs are more >> apropriate data type here. And this match-lambda rewrite will break >> downstream RDE user's setups after migrating to upstreamed version of >> service. >> >> That's why I propose to go back to pairs. >> > > I'm not opposed to going back to cons cell pairs. I didn't put too much > thought in a "list of two elements" vs. "cons cell" besides the match > statement being easier to handle with a list. > > Would this patch have unintended side effects? I thought the . in match > conditions had a different behavior. > > --8<---------------cut here---------------start------------->8--- > scheme@(guile-user)> (match '(1 2 3 4) > ((a . b) b)) > $5 = (2 3 4) > scheme@(guile-user)> (match '(1 2) > ((a . b) b)) > $6 = (2) > scheme@(guile-user)> (match '(1 . 2) > ((a . b) b)) > $7 = 2 > --8<---------------cut here---------------end--------------->8--- > > So changing to this would allow for a home-service entry like the > following to match: > > --8<---------------cut here---------------start------------->8--- > (simple-service 'my-extra-home home-service-type > `(("bob" "bill" "bertha" "billyanne" ,my-extra-home ,my-home-2))) > --8<---------------cut here---------------end--------------->8--- > > From my testing, this /will/ error (yay), but not as soon as the match > would with the current code. Instead of being caught before being passed > to the daemon, it seems to be caught while lowering the invalid > file-append object. > > Personally I would prefer to catch as many errors as possible before > beginning to pass code off to the daemon where possible. Generally > speaking it feels like pre-daemon errors are easier to mentally parse. > > In fairness, the current code also isn't trying particularly hard to > check that "user" is a string and "he" is a home-environment or printing > a fancy error message. > > Perhaps this would work? > > --8<---------------cut here---------------start------------->8--- > (define-module (gnu services guix) > ... > #:use-module (gnu home)) > ... > (define (guix-home-shepherd-service config) > (map (match-lambda > (((? string? user) . (? home-environment? he)) > (shepherd-service > ... > )) > (e (error "Invalid value for guix-home-shepherd-service: " e))) > config)) > --8<---------------cut here---------------end--------------->8--- This idea is good, I'll incorporate this into v2. > > Maybe I'm just being silly. 🙂
Hi, Andrew Tropin <andrew@trop.in> writes: > On 2024-05-23 11:38, Zheng Junjie wrote: > >> Andrew Tropin via Guix-patches via <guix-patches@gnu.org> writes: >> >>> After rewriting from car/cdr to match-lambda in v2 of this patch: >>> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard@freakingpenguin.com/ >>> >>> the format changed from pairs to lists, I didn't noticed this nuance >>> during review because the documentation still says that service should >>> be configured and extended with pairs. Also, pairs are more >>> apropriate data type here. And this match-lambda rewrite will break >>> downstream RDE user's setups after migrating to upstreamed version of >>> service. >>> >>> That's why I propose to go back to pairs. >> >> Maybe we can support pairs and list of length two at same time? > > Thank you for the idea, however I think ambiguity is a bad practice, > from my early experience with guix it's more confusing rather than > helpful. I agree.
On 2024-05-22 14:02, Andrew Tropin via Guix-patches via wrote: > After rewriting from car/cdr to match-lambda in v2 of this patch: > https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard@freakingpenguin.com/ > > the format changed from pairs to lists, I didn't noticed this nuance > during review because the documentation still says that service should > be configured and extended with pairs. Also, pairs are more > apropriate data type here. And this match-lambda rewrite will break > downstream RDE user's setups after migrating to upstreamed version of > service. > > That's why I propose to go back to pairs. > > Andrew Tropin (1): > services: home: Use pairs instead of lists. > > doc/guix.texi | 4 ++-- > gnu/services/guix.scm | 2 +- > gnu/tests/guix.scm | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > > base-commit: b06a460bf5236a9d52f936f2023451051b3e622a Merged v2 with updated API and additional type checks.
Hi Andrew, Andrew Tropin <andrew@trop.in> skribis: > On 2024-05-22 14:02, Andrew Tropin via Guix-patches via wrote: > >> After rewriting from car/cdr to match-lambda in v2 of this patch: >> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard@freakingpenguin.com/ >> >> the format changed from pairs to lists, I didn't noticed this nuance >> during review because the documentation still says that service should >> be configured and extended with pairs. Also, pairs are more >> apropriate data type here. And this match-lambda rewrite will break >> downstream RDE user's setups after migrating to upstreamed version of >> service. >> >> That's why I propose to go back to pairs. >> >> Andrew Tropin (1): >> services: home: Use pairs instead of lists. >> >> doc/guix.texi | 4 ++-- >> gnu/services/guix.scm | 2 +- >> gnu/tests/guix.scm | 2 +- >> 3 files changed, 4 insertions(+), 4 deletions(-) >> >> >> base-commit: b06a460bf5236a9d52f936f2023451051b3e622a > > Merged v2 with updated API and additional type checks. Perhaps I wasn’t clear enough when asking for clarifications¹, but I think this change shouldn’t happen: first because it’s an incompatible change that will break user configs, and second because it’s inconsistent with other similar interfaces (such as ‘authorized-keys’ and <openssh-configuration>). For these reasons, I’m in favor of reverting this change. What do others think? Aside, it’s unfortunate that you weren’t around to review this patch initially, despite being one of the recipients: <https://issues.guix.gnu.org/69781>. I think it’s important to not give the impression that you chime in just when an rde incompatibility comes up. Thanks, Ludo’. ¹ https://issues.guix.gnu.org/71111#8
On Sun, Jun 02, 2024 at 12:15:14PM +0200, Ludovic Courtès wrote: > Hi Andrew, > > Andrew Tropin <andrew@trop.in> skribis: > > > On 2024-05-22 14:02, Andrew Tropin via Guix-patches via wrote: > > > >> After rewriting from car/cdr to match-lambda in v2 of this patch: > >> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard@freakingpenguin.com/ > >> > >> the format changed from pairs to lists, I didn't noticed this nuance > >> during review because the documentation still says that service should > >> be configured and extended with pairs. Also, pairs are more > >> apropriate data type here. And this match-lambda rewrite will break > >> downstream RDE user's setups after migrating to upstreamed version of > >> service. > >> > >> That's why I propose to go back to pairs. > >> > >> Andrew Tropin (1): > >> services: home: Use pairs instead of lists. > >> > >> doc/guix.texi | 4 ++-- > >> gnu/services/guix.scm | 2 +- > >> gnu/tests/guix.scm | 2 +- > >> 3 files changed, 4 insertions(+), 4 deletions(-) > >> > >> > >> base-commit: b06a460bf5236a9d52f936f2023451051b3e622a > > > > Merged v2 with updated API and additional type checks. > > Perhaps I wasn’t clear enough when asking for clarifications¹, but I > think this change shouldn’t happen: first because it’s an incompatible > change that will break user configs, and second because it’s > inconsistent with other similar interfaces (such as ‘authorized-keys’ > and <openssh-configuration>). > > For these reasons, I’m in favor of reverting this change. > > What do others think? This patch also added home-environment? without adding an import of (gnu home). It's unfortunate that the wording for the manual says 'pair' when it's a list, but IMO that's more of a typo in the manual than a mistake in the code. With a quick look I didn't see in any of my OS configs configurations with pair notations, even with simple-service or extra-special-file, where it would have been most likely. I think it would be best to roll this back. > Aside, it’s unfortunate that you weren’t around to review this patch > initially, despite being one of the recipients: > <https://issues.guix.gnu.org/69781>. I think it’s important to not give > the impression that you chime in just when an rde incompatibility comes > up. > > Thanks, > Ludo’. > > ¹ https://issues.guix.gnu.org/71111#8 > > >
On 2024-06-02 12:15, Ludovic Courtès wrote: > Hi Andrew, > > Andrew Tropin <andrew@trop.in> skribis: > >> On 2024-05-22 14:02, Andrew Tropin via Guix-patches via wrote: >> >>> After rewriting from car/cdr to match-lambda in v2 of this patch: >>> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard@freakingpenguin.com/ >>> >>> the format changed from pairs to lists, I didn't noticed this nuance >>> during review because the documentation still says that service should >>> be configured and extended with pairs. Also, pairs are more >>> apropriate data type here. And this match-lambda rewrite will break >>> downstream RDE user's setups after migrating to upstreamed version of >>> service. >>> >>> That's why I propose to go back to pairs. >>> >>> Andrew Tropin (1): >>> services: home: Use pairs instead of lists. >>> >>> doc/guix.texi | 4 ++-- >>> gnu/services/guix.scm | 2 +- >>> gnu/tests/guix.scm | 2 +- >>> 3 files changed, 4 insertions(+), 4 deletions(-) >>> >>> >>> base-commit: b06a460bf5236a9d52f936f2023451051b3e622a >> >> Merged v2 with updated API and additional type checks. > > Perhaps I wasn’t clear enough when asking for clarifications¹, but I > think this change shouldn’t happen: first because it’s an incompatible > change that will break user configs, and second because it’s > inconsistent with other similar interfaces (such as ‘authorized-keys’ > and <openssh-configuration>). > > For these reasons, I’m in favor of reverting this change. > > What do others think? > > Aside, it’s unfortunate that you weren’t around to review this patch > initially, despite being one of the recipients: > <https://issues.guix.gnu.org/69781>. We discussed the upstreaming of this service with Richard and I was following the thread above, so I was around. I didn't merge or comment on it because it is literally code written by me, so it make sense to let someone else to review and merge it. I didn't realise that in the second revision API was changed from pairs to lists, when destructuring was rewritten from car/cdr to match. I skimmed through the docs and was satisfyed and didn't wrote anything. It came up only now, when people started reporting problems. > I think it’s important to not give the impression that you chime in > just when an rde incompatibility comes up. Not sure what you mean.
On 2024-06-02 13:37, Efraim Flashner wrote: > On Sun, Jun 02, 2024 at 12:15:14PM +0200, Ludovic Courtès wrote: >> Hi Andrew, >> >> Andrew Tropin <andrew@trop.in> skribis: >> >> > On 2024-05-22 14:02, Andrew Tropin via Guix-patches via wrote: >> > >> >> After rewriting from car/cdr to match-lambda in v2 of this patch: >> >> https://yhetil.org/guix-patches/3394b0b51f6a5a608ebcfb7a63fdc34e52fe928e.1711046203.git.richard@freakingpenguin.com/ >> >> >> >> the format changed from pairs to lists, I didn't noticed this nuance >> >> during review because the documentation still says that service should >> >> be configured and extended with pairs. Also, pairs are more >> >> apropriate data type here. And this match-lambda rewrite will break >> >> downstream RDE user's setups after migrating to upstreamed version of >> >> service. >> >> >> >> That's why I propose to go back to pairs. >> >> >> >> Andrew Tropin (1): >> >> services: home: Use pairs instead of lists. >> >> >> >> doc/guix.texi | 4 ++-- >> >> gnu/services/guix.scm | 2 +- >> >> gnu/tests/guix.scm | 2 +- >> >> 3 files changed, 4 insertions(+), 4 deletions(-) >> >> >> >> >> >> base-commit: b06a460bf5236a9d52f936f2023451051b3e622a >> > >> > Merged v2 with updated API and additional type checks. >> >> Perhaps I wasn’t clear enough when asking for clarifications¹, but I >> think this change shouldn’t happen: first because it’s an incompatible >> change that will break user configs, and second because it’s >> inconsistent with other similar interfaces (such as ‘authorized-keys’ >> and <openssh-configuration>). >> >> For these reasons, I’m in favor of reverting this change. >> >> What do others think? > > This patch also added home-environment? without adding an import of > (gnu home). > > It's unfortunate that the wording for the manual says 'pair' when it's a > list, but IMO that's more of a typo in the manual than a mistake in the > code. > > With a quick look I didn't see in any of my OS configs configurations > with pair notations, even with simple-service or extra-special-file, > where it would have been most likely. > > I think it would be best to roll this back. ok, reverted. > >> Aside, it’s unfortunate that you weren’t around to review this patch >> initially, despite being one of the recipients: >> <https://issues.guix.gnu.org/69781>. I think it’s important to not give >> the impression that you chime in just when an rde incompatibility comes >> up. >> >> Thanks, >> Ludo’. >> >> ¹ https://issues.guix.gnu.org/71111#8 >> >> >>