From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jelle Licht Subject: Re: Maintaining implementations of similar utility functions like json-fetch Date: Wed, 31 Jan 2018 18:32:21 +0100 Message-ID: References: <87po5v1dy0.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="f4030437d4dca1c4fb056415dc50" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:40890) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1egwFV-0001MP-OO for guix-devel@gnu.org; Wed, 31 Jan 2018 12:32:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1egwFU-0002B0-GR for guix-devel@gnu.org; Wed, 31 Jan 2018 12:32:41 -0500 In-Reply-To: <87po5v1dy0.fsf@gnu.org> List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: =?UTF-8?Q?Ludovic_Court=C3=A8s?= Cc: guix-devel --f4030437d4dca1c4fb056415dc50 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi Ludo', 2018-01-27 17:09 GMT+01:00 Ludovic Court=C3=A8s : > 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 (gui= x > > 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=E2=80=99s 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 tr= y > 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=E2=80=99t know that a util= ity > function is available so you end up writing your own, and maybe the > reviewers don=E2=80=99t notice either and it goes through; or sometimes y= ou 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=E2=80=99. > - Jelle --f4030437d4dca1c4fb056415dc50 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Ludo',


2018-01-27 17:09 GMT+01:00 Ludovic Court=C3=A8s <ludo@gnu.= org>:
Hello!

Jelle Licht <jlicht@fsfe.org> = skribis:

> I noticed that there are currently two very similar functions for fetc= hing
> 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 fo= rmat
> 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.=C2=A0 It=E2=80=99s not even cle= ar how we ended
up with the second one.

I even had a th= ird one in my local tree which happened to have a conflict, which
is ho= w I found out in the first place, so I understand how these things can happ= en.

> 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 t= ry 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.=C2=A0 Sometimes you just don=E2=80=99t know that a u= tility
function is available so you end up writing your own, and maybe the
reviewers don=E2=80=99t 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 `js= on-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 o= f the
existing usages of `json-fetch*' expect an alist in= stead. 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 als= o not be
what we want.


Thanks,
Ludo=E2=80=99.

- Jelle
--f4030437d4dca1c4fb056415dc50--