Re: Maintaining implementations of similar utility functions like json-fetch
Hello Jelle! Jelle Licht skribis: > Sorry for the delay. Cue the drum roll; Attached is my initial draft of > this patch. I initially wanted to split it up into 2 or more patches, but > could not make this work in a way that I could wrap my head around. Better late than never! :-) It looks good to me. > Also, there is yet another 'json-fetch'-like function implemented in > `guix/ci.scm', but I was not sure whether the error-handling facilities > would be applicable there. I’d leave (guix ci) as-is. It’s used by (guix scripts weather), which has custom HTTP error handling in this case. > From c60686975df206118c3a26cc9c2cef2a93b2 Mon Sep 17 00:00:00 2001 > From: Jelle Licht > Date: Sun, 10 Jun 2018 20:35:39 +0200 > Subject: [PATCH] import: json: Consolidate duplicate json-fetch functionality. > > * guix/import/json.scm (json-fetch): Return a list or hash table. > (json-fetch-alist): New procedure. > * guix/import/github.scm (json-fetch*): Remove. > (latest-released-version): Use json-fetch. > * guix/import/cpan.scm (module->dist-name): Use json-fetch-alist. > (cpan-fetch): Likewise. > * guix/import/crate.scm (crate-fetch): Likewise. > * guix/import/gem.scm (rubygems-fetch): Likewise. > * guix/import/pypi.scm (pypi-fetch): Likewise. > * guix/import/stackage.scm (stackage-lts-info-fetch): Likewise. LGTM! > --- a/guix/import/json.scm > +++ b/guix/import/json.scm > @@ -22,15 +22,25 @@ >#:use-module (guix http-client) >#:use-module (guix import utils) >#:use-module (srfi srfi-34) > - #:export (json-fetch)) > + #:export (json-fetch > +json-fetch-alist)) > > (define (json-fetch url) > - "Return an alist representation of the JSON resource URL, or #f on > failure." > + "Return a representation of the JSON resource URL (a list or hash table), > or > +#f if URL returns 403 or 404." >(guard (c ((and (http-get-error? c) > - (= 404 (http-get-error-code c))) > - #f)) ;"expected" if package is unknown > -(let* ((port (http-fetch url #:headers '((user-agent . "GNU Guile") > - (Accept . "application/json" > - (result (hash-table->alist (json->scm port > + (let ((error (http-get-error-code c))) > +(or (= 403 error) > +(= 404 error > + #f)) > +;; Note: many websites returns 403 if we omit a 'User-Agent' header. > +(let* ((port (http-fetch url #:headers '((user-agent . "GNU Guile") > + (Accept . > "application/json" > + (result (json->scm port))) >(close-port port) >result))) > + > +(define (json-fetch-alist url) > + "Return an alist representation of the JSON resource URL, or #f if URL > +returns 403 or 404." > + (hash-table->alist (json-fetch url))) There’s an issue: some of the values in the hash tables may themselves be hash tables as well, so we’d need to convert them as well. The problem was already there though (and apparently it’s not much of a problem :-)), so we can address it later. Thank you! Ludo’.
Re: Maintaining implementations of similar utility functions like json-fetch
Ludovic Courtès writes: > Hey, > > Jelle Licht skribis: > >> I basically added the robust features of `json-fetch*' to the exported >> `json-fetch' >> instead, and all existing functionality seems to work out as far as I can >> see. > > So are you saying that we can get rid of ‘json-fetch*’? > >> I did notice that I now produce hash-tables by default, and some of the >> existing usages of `json-fetch*' expect an alist instead. What would be a >> guile- >> appropriate way of dealing with this? I currently have multiple >> `(hash-table->alist (json-fetch <...>))' littered in my patch which seems >> suboptimal, >> but always converting the parsed json into an alist seems like it might >> also not be >> what we want. > > Why insist on having an alist? Perhaps you can just stick to hash > tables? :-) > > Ludo’. Hey hey, Sorry for the delay. Cue the drum roll; Attached is my initial draft of this patch. I initially wanted to split it up into 2 or more patches, but could not make this work in a way that I could wrap my head around. Also, there is yet another 'json-fetch'-like function implemented in `guix/ci.scm', but I was not sure whether the error-handling facilities would be applicable there. Anyway, I am open to comments. I have verified that at least the (tests of the) importers still work as they did before. After the comments, I could push it myself if that is okay. From c60686975df206118c3a26cc9c2cef2a93b2 Mon Sep 17 00:00:00 2001 From: Jelle Licht Date: Sun, 10 Jun 2018 20:35:39 +0200 Subject: [PATCH] import: json: Consolidate duplicate json-fetch functionality. * guix/import/json.scm (json-fetch): Return a list or hash table. (json-fetch-alist): New procedure. * guix/import/github.scm (json-fetch*): Remove. (latest-released-version): Use json-fetch. * guix/import/cpan.scm (module->dist-name): Use json-fetch-alist. (cpan-fetch): Likewise. * guix/import/crate.scm (crate-fetch): Likewise. * guix/import/gem.scm (rubygems-fetch): Likewise. * guix/import/pypi.scm (pypi-fetch): Likewise. * guix/import/stackage.scm (stackage-lts-info-fetch): Likewise. --- guix/import/cpan.scm | 9 + guix/import/crate.scm| 4 ++-- guix/import/gem.scm | 2 +- guix/import/github.scm | 19 ++- guix/import/json.scm | 24 +--- guix/import/pypi.scm | 4 ++-- guix/import/stackage.scm | 2 +- 7 files changed, 30 insertions(+), 34 deletions(-) diff --git a/guix/import/cpan.scm b/guix/import/cpan.scm index 58c051e28..08bed8767 100644 --- a/guix/import/cpan.scm +++ b/guix/import/cpan.scm @@ -88,9 +88,10 @@ "Return the base distribution module for a given module. E.g. the 'ok' module is distributed with 'Test::Simple', so (module->dist-name \"ok\") would return \"Test-Simple\"" - (assoc-ref (json-fetch (string-append "https://fastapi.metacpan.org/v1/module/"; -module -"?fields=distribution")) + (assoc-ref (json-fetch-alist (string-append +"https://fastapi.metacpan.org/v1/module/"; +module +"?fields=distribution")) "distribution")) (define (package->upstream-name package) @@ -113,7 +114,7 @@ return \"Test-Simple\"" "Return an alist representation of the CPAN metadata for the perl module MODULE, or #f on failure. MODULE should be e.g. \"Test::Script\"" ;; This API always returns the latest release of the module. - (json-fetch (string-append "https://fastapi.metacpan.org/v1/release/"; name))) + (json-fetch-alist (string-append "https://fastapi.metacpan.org/v1/release/"; name))) (define (cpan-home name) (string-append "http://search.cpan.org/dist/"; name "/")) diff --git a/guix/import/crate.scm b/guix/import/crate.scm index a7485bb4d..3724a457a 100644 --- a/guix/import/crate.scm +++ b/guix/import/crate.scm @@ -51,7 +51,7 @@ (define (crate-kind-predicate kind) (lambda (dep) (string=? (assoc-ref dep "kind") kind))) - (and-let* ((crate-json (json-fetch (string-append crate-url crate-name))) + (and-let* ((crate-json (json-fetch-alist (string-append crate-url crate-name))) (crate (assoc-ref crate-json "crate")) (name (assoc-ref crate "name")) (version (assoc-ref crate "max_version")) @@ -63,7 +63,7 @@ string->license) '())) ;missing license info (path (string-append "/" version "/dependencies")) - (deps-json (json-fetch (string-append crate-url name path))) + (deps-json (json-fetch-alist (string-append crate-url name path))) (deps (assoc-ref deps-json "dependencies")) (input-crates (filter (crate-kind-predicate "normal") deps)) (native-input-crates diff --git a/guix/import/gem.scm b/guix/import/gem.scm index 6e914d629..646163fb7 100
Re: Maintaining implementations of similar utility functions like json-fetch
l...@gnu.org (Ludovic Courtès) writes: > Hey, > > Jelle Licht skribis: > >> I basically added the robust features of `json-fetch*' to the exported >> `json-fetch' >> instead, and all existing functionality seems to work out as far as I can >> see. > > So are you saying that we can get rid of ‘json-fetch*’? > >> I did notice that I now produce hash-tables by default, and some of the >> existing usages of `json-fetch*' expect an alist instead. What would be a >> guile- >> appropriate way of dealing with this? I currently have multiple >> `(hash-table->alist (json-fetch <...>))' littered in my patch which seems >> suboptimal, >> but always converting the parsed json into an alist seems like it might >> also not be >> what we want. > > Why insist on having an alist? Perhaps you can just stick to hash > tables? :-) > We also have vhashes... > Ludo’.
Re: Maintaining implementations of similar utility functions like json-fetch
Hey, Jelle Licht skribis: > I basically added the robust features of `json-fetch*' to the exported > `json-fetch' > instead, and all existing functionality seems to work out as far as I can > see. So are you saying that we can get rid of ‘json-fetch*’? > I did notice that I now produce hash-tables by default, and some of the > existing usages of `json-fetch*' expect an alist instead. What would be a > guile- > appropriate way of dealing with this? I currently have multiple > `(hash-table->alist (json-fetch <...>))' littered in my patch which seems > suboptimal, > but always converting the parsed json into an alist seems like it might > also not be > what we want. Why insist on having an alist? Perhaps you can just stick to hash tables? :-) Ludo’.
Re: Maintaining implementations of similar utility functions like json-fetch
2018-02-01 12:54 GMT+01:00 Catonano : > > > 2018-01-31 18:32 GMT+01:00 Jelle Licht : > >> Hi Ludo', >> >> >> 2018-01-27 17:09 GMT+01:00 Ludovic Courtès : >> >>> Hello! >>> >>> Jelle Licht skribis: >>> >>> > I noticed that there are currently two very similar functions for >>> fetching >>> > json data; `json-fetch' in (guix import json) and `json-fetch*' in >>> (guix >>> > import github). >>> > >>> > Some things I noticed: >>> > - Dealing with http error codes seems to be a bit more robust in >>> > `json-fetch*'. >>> > - Making sure that (compliant) servers give responses in the proper >>> format >>> > seems more robust in `json-fetch' due to using Accept headers. >>> > - Dealing with the fact that json responses are technically allowed to >>> be >>> > lists of objects, which `json-fetch' does not handle gracefully. >>> > >>> > For this issue specifically, would it make sense to combine the two >>> > definitions into a more general one? >>> >>> Definitely, we should just keep one. It’s not even clear how we ended >>> up with the second one. >>> >> >> I even had a third one in my local tree which happened to have a >> conflict, which >> is how I found out in the first place, so I understand how these things >> can happen. >> >>> >>> > My more general concern would be on how we can prevent bug fixes only >>> being >>> > applied to one of several nearly identical functions. IOW, should we >>> try to >>> > prevent situations like this from arising, or is it okay if we somehow >>> make >>> > sure that fixes should be applied to both locations? >>> >>> We should prevent such situations from arising, and I think we do. >>> >>> Regarding this could we consider implementing on of these funtions in terms of each other, or use the common generic one to implent both, as this would mitigate the problem where only one of these is fixed. > The difficulty is that avoiding duplication requires knowing the whole >>> code base well enough. Sometimes you just don’t know that a utility >>> function is available so you end up writing your own, and maybe the >>> reviewers don’t notice either and it goes through; or sometimes you need >>> a slightly different version so you duplicate the function instead of >>> generalizing it. >>> >>> Anyway, when we find occurrences of this pattern, we should fix them! >>> >> >> I basically added the robust features of `json-fetch*' to the exported >> `json-fetch' >> instead, and all existing functionality seems to work out as far as I can >> see. >> >> I did notice that I now produce hash-tables by default, and some of the >> existing usages of `json-fetch*' expect an alist instead. What would be a >> guile- >> appropriate way of dealing with this? I currently have multiple >> `(hash-table->alist (json-fetch <...>))' littered in my patch which seems >> suboptimal, >> but always converting the parsed json into an alist seems like it might >> also not be >> what we want. >> > > of course I' d wait for a thought by some more competent guiler, but I' d > like to offer my take on this > > The new function could take one further argument, a boolean > > If the boolean is true, it could return a hash table > > Otherwise, it could return a list > > If the majority of call sites expect a list, the further argument could > set to false as default > > So you' d only have to fix those call sites that want a hash table instead > > If, instead, the majority of call sites want a hash table, your procedure > would return a hash table by default and a list by a further argument, so > you' d have to fix a minority of call sites anyway > > I hope I didn' t make myself a fool :-/ > > >> Thanks, >> Ludo’. >> > >> - Jelle >> > >
Re: Maintaining implementations of similar utility functions like json-fetch
2018-01-31 18:32 GMT+01:00 Jelle Licht : > Hi Ludo', > > > 2018-01-27 17:09 GMT+01:00 Ludovic Courtès : > >> Hello! >> >> Jelle Licht skribis: >> >> > I noticed that there are currently two very similar functions for >> fetching >> > json data; `json-fetch' in (guix import json) and `json-fetch*' in (guix >> > import github). >> > >> > Some things I noticed: >> > - Dealing with http error codes seems to be a bit more robust in >> > `json-fetch*'. >> > - Making sure that (compliant) servers give responses in the proper >> format >> > seems more robust in `json-fetch' due to using Accept headers. >> > - Dealing with the fact that json responses are technically allowed to >> be >> > lists of objects, which `json-fetch' does not handle gracefully. >> > >> > For this issue specifically, would it make sense to combine the two >> > definitions into a more general one? >> >> Definitely, we should just keep one. It’s not even clear how we ended >> up with the second one. >> > > I even had a third one in my local tree which happened to have a conflict, > which > is how I found out in the first place, so I understand how these things > can happen. > >> >> > My more general concern would be on how we can prevent bug fixes only >> being >> > applied to one of several nearly identical functions. IOW, should we >> try to >> > prevent situations like this from arising, or is it okay if we somehow >> make >> > sure that fixes should be applied to both locations? >> >> We should prevent such situations from arising, and I think we do. >> >> The difficulty is that avoiding duplication requires knowing the whole >> code base well enough. Sometimes you just don’t know that a utility >> function is available so you end up writing your own, and maybe the >> reviewers don’t notice either and it goes through; or sometimes you need >> a slightly different version so you duplicate the function instead of >> generalizing it. >> >> Anyway, when we find occurrences of this pattern, we should fix them! >> > > I basically added the robust features of `json-fetch*' to the exported > `json-fetch' > instead, and all existing functionality seems to work out as far as I can > see. > > I did notice that I now produce hash-tables by default, and some of the > existing usages of `json-fetch*' expect an alist instead. What would be a > guile- > appropriate way of dealing with this? I currently have multiple > `(hash-table->alist (json-fetch <...>))' littered in my patch which seems > suboptimal, > but always converting the parsed json into an alist seems like it might > also not be > what we want. > of course I' d wait for a thought by some more competent guiler, but I' d like to offer my take on this The new function could take one further argument, a boolean If the boolean is true, it could return a hash table Otherwise, it could return a list If the majority of call sites expect a list, the further argument could set to false as default So you' d only have to fix those call sites that want a hash table instead If, instead, the majority of call sites want a hash table, your procedure would return a hash table by default and a list by a further argument, so you' d have to fix a minority of call sites anyway I hope I didn' t make myself a fool :-/ > Thanks, > Ludo’. > > - Jelle >
Re: Maintaining implementations of similar utility functions like json-fetch
Hi Ludo', 2018-01-27 17:09 GMT+01:00 Ludovic Courtès : > Hello! > > Jelle Licht skribis: > > > I noticed that there are currently two very similar functions for > fetching > > json data; `json-fetch' in (guix import json) and `json-fetch*' in (guix > > import github). > > > > Some things I noticed: > > - Dealing with http error codes seems to be a bit more robust in > > `json-fetch*'. > > - Making sure that (compliant) servers give responses in the proper > format > > seems more robust in `json-fetch' due to using Accept headers. > > - Dealing with the fact that json responses are technically allowed to be > > lists of objects, which `json-fetch' does not handle gracefully. > > > > For this issue specifically, would it make sense to combine the two > > definitions into a more general one? > > Definitely, we should just keep one. It’s not even clear how we ended > up with the second one. > I even had a third one in my local tree which happened to have a conflict, which is how I found out in the first place, so I understand how these things can happen. > > > My more general concern would be on how we can prevent bug fixes only > being > > applied to one of several nearly identical functions. IOW, should we try > to > > prevent situations like this from arising, or is it okay if we somehow > make > > sure that fixes should be applied to both locations? > > We should prevent such situations from arising, and I think we do. > > The difficulty is that avoiding duplication requires knowing the whole > code base well enough. Sometimes you just don’t know that a utility > function is available so you end up writing your own, and maybe the > reviewers don’t notice either and it goes through; or sometimes you need > a slightly different version so you duplicate the function instead of > generalizing it. > > Anyway, when we find occurrences of this pattern, we should fix them! > I basically added the robust features of `json-fetch*' to the exported `json-fetch' instead, and all existing functionality seems to work out as far as I can see. I did notice that I now produce hash-tables by default, and some of the existing usages of `json-fetch*' expect an alist instead. What would be a guile- appropriate way of dealing with this? I currently have multiple `(hash-table->alist (json-fetch <...>))' littered in my patch which seems suboptimal, but always converting the parsed json into an alist seems like it might also not be what we want. > Thanks, > Ludo’. > - Jelle
Re: Maintaining implementations of similar utility functions like json-fetch
Hello! Jelle Licht skribis: > I noticed that there are currently two very similar functions for fetching > json data; `json-fetch' in (guix import json) and `json-fetch*' in (guix > import github). > > Some things I noticed: > - Dealing with http error codes seems to be a bit more robust in > `json-fetch*'. > - Making sure that (compliant) servers give responses in the proper format > seems more robust in `json-fetch' due to using Accept headers. > - Dealing with the fact that json responses are technically allowed to be > lists of objects, which `json-fetch' does not handle gracefully. > > For this issue specifically, would it make sense to combine the two > definitions into a more general one? Definitely, we should just keep one. It’s not even clear how we ended up with the second one. > My more general concern would be on how we can prevent bug fixes only being > applied to one of several nearly identical functions. IOW, should we try to > prevent situations like this from arising, or is it okay if we somehow make > sure that fixes should be applied to both locations? We should prevent such situations from arising, and I think we do. The difficulty is that avoiding duplication requires knowing the whole code base well enough. Sometimes you just don’t know that a utility function is available so you end up writing your own, and maybe the reviewers don’t notice either and it goes through; or sometimes you need a slightly different version so you duplicate the function instead of generalizing it. Anyway, when we find occurrences of this pattern, we should fix them! Thanks, Ludo’.
Maintaining implementations of similar utility functions like json-fetch
Hello! I noticed that there are currently two very similar functions for fetching json data; `json-fetch' in (guix import json) and `json-fetch*' in (guix import github). Some things I noticed: - Dealing with http error codes seems to be a bit more robust in `json-fetch*'. - Making sure that (compliant) servers give responses in the proper format seems more robust in `json-fetch' due to using Accept headers. - Dealing with the fact that json responses are technically allowed to be lists of objects, which `json-fetch' does not handle gracefully. For this issue specifically, would it make sense to combine the two definitions into a more general one? My more general concern would be on how we can prevent bug fixes only being applied to one of several nearly identical functions. IOW, should we try to prevent situations like this from arising, or is it okay if we somehow make sure that fixes should be applied to both locations? -- Jelle
