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

[Gábor Boskovits]

2018-02-01 12:54 GMT+01:00 Catonano <address@hidden>:

2018-01-31 18:32 GMT+01:00 Jelle Licht <address@hidden>:

Hi Ludo',

2018-01-27 17:09 GMT+01:00 Ludovic Courtès <address@hidden>:

Hello!

Jelle Licht <address@hidden> 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