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.
>
> 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