Bug#806740: tracker.debian.org: Please add AppStream hints panel
2016-05-20 17:29 GMT+02:00 Raphael Hertzog <hertzog@debian.org>:
> Hi Matthias,
>
> On Wed, 11 May 2016, Matthias Klumpp wrote:
>> I created a preliminary patch to solve this issue.
>
> Thanks, here are some early comments, I did not review everything in
> detail. The patch seems rather long for this feature alone...
Half of it is tests :P
>> +++ b/distro_tracker/vendor/debian/migrations/0002_appstreamstats.py
>
>> --- a/distro_tracker/vendor/debian/models.py
>> +++ b/distro_tracker/vendor/debian/models.py
>> @@ -89,6 +89,51 @@ def get_lintian_url(self, full=False):
>>
>>
>> @python_2_unicode_compatible
>> +class AppStreamStats(models.Model):
>> + """
>> + Model for AppStream hint stats of packages.
>> + """
>> + package = models.OneToOneField(PackageName, related_name='appstream_stats')
>> + stats = JSONField()
>
> We don't want this model. It's basically a specialized version
> of PackageExtractedInfo. Please use PackageExtractedInfo to store
> your data related to each package (with a key="appstream" or similar).
Okay - I modeled this after the Lintian bits, so maybe Lintian should
use that model too?
>> [...]
>> + # TODO: What is the proper way to get (guess?) the archive-component vai the source-pkg here?
>> + section = "main"
>
> I see two ways to achieve this:
>
> - either by parsing the section in SourcePackageRepositoryEntry
> component = "main"
> if "/" in section:
> component = section.split("/")[0]
That sounds like a good solution, thank you!
>> +++ b/distro_tracker/vendor/debian/templates/debian/appstream-action-item.html
>> @@ -0,0 +1,14 @@
>> +{% with warnings=item.extra_data.warnings %}
>> +{% with errors=item.extra_data.errors %}
>> +<a href="https://wiki.debian.org/AppStream";>AppStream</a> found
>> +<a href="{{ item.extra_data.appstream_url }}">
>> +{% if errors %}
>> +<span>{{ errors }} error{% if errors > 1 %}s{% endif %}</span>
>> +{% if warnings %}and{% endif %}
>> +{% endif %}
>> +{% if warnings %}
>> +<span>{{ warnings }} warning{% if warnings > 1 %}s{% endif %}</span>
>> +{% endif %}
>
> Django has a pluralize filter for this kind of problems:
> https://docs.djangoproject.com/en/1.9/ref/templates/builtins/#pluralize
Neat! (Maybe something the Lintian code should use too then ^^)
>> --- /dev/null
>> +++ b/distro_tracker/vendor/debian/templates/debian/appstream-link.html
>> @@ -0,0 +1,10 @@
>
> I don't think that you should use a link and an action item for the same
> purpose. I would keep only the action item.
>
> About the tests, they might be a bit too extensive or they need some
> refactoring to avoid duplication, I haven't checked in details.
It's (almost) the same tests used for the Lintian code, which I though
were very extensive too, but on the other hand nobody has ever
complained about too many tests - until now ^^
> I think that gzip compression should be abstracted away and
> be tested separately. In other words, the mocked function that you use
> should provide uncompressed data directly.
That makes sense, I'll look into it.
> Also you might want to use static data stored in "tests-data" instead
> of duplicating the same data in each test. We have a helper method
> to locate data files from those directories (get_test_data_path).
That would make sense for longer data which really is the same - most
of the JSON snippets used in the tests have some variations.
I'll go through that again.
> I think this is enough feedback for a start. Thanks for working on this!
Thanks for reviewing it!
2016-05-21 5:53 GMT+02:00 Paul Wise <pabs@debian.org>:
> On Fri, May 20, 2016 at 11:29 PM, Raphael Hertzog wrote:
>
>> I don't think that you should use a link and an action item for the same
>> purpose. I would keep only the action item.
>
> I think the link in the tracker should point at the metainfo pages and
> those metainfo pages should point at the issues pages when there are
> issues.
>
> https://appstream.debian.org/sid/main/metainfo/cellwriter.html
> https://appstream.debian.org/sid/main/issues/cellwriter.html
Yes, that makes sense. Linking the metainfo page to the issues page is
something on my todo list already, and having the tracker link point
at the metainfo page makes a lot of sense. Unfortunately, one source
package might have multiple binary packages with different metadata,
and asgen only operates on binary packages.
So, we would need to point the link at the maintainer's package/metainfo list:
https://appstream.debian.org/sid/main/metainfo/index.html#Michael_Levin_%3Crisujin@risujin.org%3E
Cheers,
Matthias
--
I welcome VSRE emails. See http://vsre.info/
Reply to: