Code review comment for lp:~paulliu/unity-scope-click/showratings

Revision history for this message
dobey (dobey) wrote :

> > + return get_base_url() +
> > "/api/1.0/reviews/filter/%s/%s/%s/%s/%s%s/page/%s/%s/";
> >
> Hrm - I assume vala/C++ printf doesn't support a hash of args so you can
> return something like: ".../filter/%(lang)s/%(origin)s/%(distroseries)s/...")?
> That would be clearer.

I don't think it would be made clearer. The most confusing part was the "%s%s" (without a slash between them), which we agree is wrong. I think a simple comment that explains the structure of the URL would be enough to clarify what each %s is, in the string. The Python string formatting would obviously be better, but we can't do that in C/Vala, so a comment would be well enough.

> Correct, the origin should be "ubuntu" here, while the distroseries should be
> "click" (there is no natty/precise/etc. for click packages - at least, that's
> the decision that was made for exporting the apps from devportal [2]:
>
> [2] http://software-center.ubuntu.com/api/2.0/applications/en/ubuntu/click/
>
> I don't see why this unity-scope wouldn't just use the default for
> ReviewFilter of ubuntu/click?

Well, this doesn't make sense to me either. The origin isn't Ubuntu (ie, these packages aren't part of the Ubuntu archive, but are published by other individuals or organizations). So, I don't think we should be defaulting to anything, but instead getting some value from the details of the package, and using that as the origin. Maybe we need to add something to the package details endpoint of the click-package-index API to do that. Also, I think the distroseries does need to be the actual distroseries here. But maybe we should be using the "click_framework" value from the package details here, rather than saucy/trusty/etc… ?

« Back to merge proposal