From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?G=C3=A1bor_Boskovits?= Subject: Re: Maintaining implementations of similar utility functions like json-fetch Date: Thu, 1 Feb 2018 13:14:47 +0100 Message-ID: References: <87po5v1dy0.fsf@gnu.org> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="001a1140ba96be93190564258a89" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:52734) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ehDlS-0005w8-HC for guix-devel@gnu.org; Thu, 01 Feb 2018 07:14:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ehDlQ-0000W2-TP for guix-devel@gnu.org; Thu, 01 Feb 2018 07:14:50 -0500 Received: from mail-io0-x235.google.com ([2607:f8b0:4001:c06::235]:45335) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1ehDlQ-0000Uv-KV for guix-devel@gnu.org; Thu, 01 Feb 2018 07:14:48 -0500 Received: by mail-io0-x235.google.com with SMTP id p188so18933089ioe.12 for ; Thu, 01 Feb 2018 04:14:48 -0800 (PST) In-Reply-To: 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: Catonano Cc: guix-devel --001a1140ba96be93190564258a89 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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=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 >>> (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 t= o >>> 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 w= e 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 someho= w >>> 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=E2=80=99t know that a ut= ility >>> 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 `json-fetch*' to the exported >> `json-fetch' >> instead, and all existing functionality seems to work out as far as I ca= n >> 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 seem= s >> 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 instea= d > > 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=E2=80=99. >> > >> - Jelle >> > > --001a1140ba96be93190564258a89 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
2018= -02-01 12:54 GMT+01:00 Catonano <catonano@gmail.com>:


2018-01-31 18:32 GM= T+01:00 Jelle Licht <jlicht@fsfe.org>:
Hi Ludo',


2018-01-27 17:09 GMT+01:00 Ludovic Co= urt=C3=A8s <ludo@gnu.org>:
Hell= o!

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 h= ad 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 c= an 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 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.
Regarding this could we consider implementing o= n of these funtions in terms of each other, or
use the common gen= eric 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.=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 `json-fetch*' to the exported `json-fetch'
instea= d, 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 a= list instead. What would be a guile-
appropriate way of deali= ng with this? I currently have multiple
`(hash-table->alist (json-fe= tch <...>))' littered in my patch which seems suboptimal,
but always converting the parsed json into an alist seems like it mi= ght also not be
what we want.

of course I' d wait for a thou= ght by some more competent guiler,=C2=A0 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 has= h 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 w= ould return a hash table by default and a list by a further argument, so yo= u' d have to fix a minority of call sites anyway

I hope I didn&#= 39; t make myself a fool :-/


Thanks,
Ludo=E2=80=99.

- J= elle


--001a1140ba96be93190564258a89--