Re: Maintaining implementations of similar utility functions like json-fetch

["Gábor Boskovits" <boskovits@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3884 bytes --]

2018-02-01 12:54 GMT+01:00 Catonano <catonano@gmail.com>:

>
>
> 2018-01-31 18:32 GMT+01:00 Jelle Licht <jlicht@fsfe.org>:
>
>> Hi Ludo',
>>
>>
>> 2018-01-27 17:09 GMT+01:00 Ludovic Courtès <ludo@gnu.org>:
>>
>>> Hello!
>>>
>>> Jelle Licht <jlicht@fsfe.org> 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
>>
>
>

[-- Attachment #2: Type: text/html, Size: 6357 bytes --]