Improve "Pub Points" on pub.dev

[MohamedFBoussaid]

The current version has the following score

Maybe we can improve the score, by following pub.dev recommandations.

[stephanegigandet]

That's a great idea.

[monsieurtanuki]

Currently working on the "Pass static analysis" score (for 0.3.15+2: 20/30)

[MohamedFBoussaid]

Thanks @monsieurtanuki

[monsieurtanuki]

Hi @PrimaelQuemerais!
There's something fishy in your code in SearchResult.dart (compared to the generated SearchResult.g.dart)
The expected (and previous version) of method fromJson is:

factory SearchResult.fromJson(Map<String, dynamic> json) =>
    _$SearchResultFromJson(json);

But you changed it months ago, and now _$SearchResultFromJson(json) is never called. And its code is slightly different from the current implementation of factory SearchResult.fromJson(Map<String, dynamic> json).
In the pub.dev score, we get a 10 point penalty because _$SearchResultFromJson(json) is never used. And it's also strange that the code is different. Could you please have a look at it? If you do that I'll give you 10 points :)

[monsieurtanuki]

Hi @Grumpf!
In HttpHelper.dart you use List<int> fileBytes = await File.fromUri(entry.value).readAsBytes(); to get the content of a Uri. Nothing that really shocks me, except that using File means using dart.io, which prevents developer from using openfoodfacts with flutter web. And it costs us 10 points.
I'm sure there's another way to do that; what about this:

http.Response response = await http.get(entry.value);
List<int> fileBytes = response?.bodyBytes;

Could you please have a look at it? If you do that I'll give you 10 points, fair is fair :)

[monsieurtanuki]

Well, not everything is clear in my mind now, but we're getting closer to 110 points.

We lose 10 points because of this: "At least 20% of the public API members contain API documentation."

And there's something I don't get about http: ^1.3.0 that we're supposed to support though it's not the stable version... 10 points again...

For the record:

• pana: A library for analyzing Dart packages
• help about scoring

[PrimaelQuemerais]

Hi @monsieurtanuki,
If I recall I had to overwrite the generated factory in order to keep the products in the Json format as well as a list of objects. The json_annotation plugin doesn't allow to use a field multiple times. There is maybe a way to do it but I didn't find it at the time.
Also I can't remember why we had to keep both formats, maybe it is not the case anymore

[monsieurtanuki]

Thank you @PrimaelQuemerais for your answer.
I don't know either why both formats are kept ("for historical reasons"), but anyway it takes space to keep both, given that we can compute the json string from the json structure (the latter being the more interesting version I guess).

I think the correct way out of this would be

• to consider final List<Product> products as the only interesting field (from/to json)
• to transform final List<dynamic> jsonProducts into a deprecated getter
• or to get rid of the json annotation for this class, but I'm sure @peterwvj would not agree with that ;)

[peterwvj]

The right thing to do would be to get rid of jsonProducts. It seems wrong to have it. I noticed that it is being used in openfoodfacts.dart. I'm not sure why but it looks odd (I marked the line in the code snippet below):

  static Future<SearchResult> queryPnnsGroup(
      User user, PnnsGroupQueryConfiguration configuration,
      {QueryType queryType = QueryType.PROD}) async {
    const outputFormat = OutputFormat(format: Format.JSON);
    var queryParameters = configuration.getParametersMap();
    queryParameters[outputFormat.getName()] = outputFormat.getValue();

    var searchUri = Uri(
        scheme: URI_SCHEME,
        host: queryType == QueryType.PROD ? URI_PROD_HOST : URI_TEST_HOST,
        path: '/pnns-group-2/${configuration.group.id}/${configuration.page}',
        queryParameters: queryParameters);

    Response response = await HttpHelper()
        .doGetRequest(searchUri, user: user, queryType: queryType);
    var result = SearchResult.fromJson(json.decode(response.body));

    if (configuration.fields
            .contains(ProductField.CATEGORIES_TAGS_TRANSLATED) ||
        configuration.fields.contains(ProductField.LABELS_TAGS_TRANSLATED) ||
        configuration.fields.contains(ProductField.ALL)) {
      result.products.asMap().forEach((index, product) {
        ProductHelper.removeImages(product, configuration.language);
        ProductHelper.addTranslatedFields(product,
            result.jsonProducts.elementAt(index), configuration.language); // USED HERE !!!!!!!!!
      });
    } else {
      result.products.asMap().forEach((index, product) {
        ProductHelper.removeImages(product, configuration.language);
      });
    }

    return result;
  }

[monsieurtanuki]

A zoom on ProductHelper.addTranslatedFields:

  static void addTranslatedFields(Product product, Map<String, dynamic> source,
      OpenFoodFactsLanguage language) {
    product.categoriesTagsTranslated =
        source['categories_tags_${language.code}'];
    product.labelsTagsTranslated = source['labels_tags_${language.code}'];
  }

The idea seems to be: only if some flags are set (CATEGORIES_TAGS_TRANSLATED, LABELS_TAGS_TRANSLATED, ALL), there's an additional step that computes 2 translated fields (according to the language in parameter) (that we already know when we run the search).
The problem is that we keep the double of the needed space of each Product (around 10Kb) just for that...
A solution would be to store all the 'categories_tags_${language.code}' when we decode the json, in a Map. Then, we'll be able to retrieve the labels for each desired language, from that Map. That would cost less in terms of space.

[peterwvj]

Thanks for looking into this. Yes, that would be much better than storing all the products twice.

[monsieurtanuki]

Of course I could easily add that Map in Product.dart, but I would do that my way, which means removing the json annotations.

@peterwvj Could you add (and populate) that Map with minor changes while keeping the json annotations? Are those annotations flexible enough?

[peterwvj]

Feel free to remove the annotation from SearchResult, if you prefer.

[monsieurtanuki]

@peterwvj The action must be more specifically on Product, where the categories and their possibly translated labels are. If I only remove the field SearchResult.jsonProducts I lose the translations.

[peterwvj]

Okay, thanks for clarifying. Product.dart is huge and without json_serializable JSON conversion for this class would require a lot of manually written code, I think. I not sure it's worthwhile to be honest.

[monsieurtanuki]

@peterwvj Json annotations are unfortunately:

• not flexible enough for our needs in Product
• force us to waste space at runtime (about additional 10Kb per Product) (and where I come from it's a lot)
• blur code clarity in SearchResult

We'll still enjoy json annotations for other classes, and we'll use what json annotations generated for Product in the one-shot transition.

[peterwvj]

That's fine. By all means remove it if everyone else agrees. It's not that I'm a fan of json_serializable I'm just pointing out a significant cost of not using it. I always felt that JSON conversion is a bit awkward in Dart as compared to languages that support reflection.

[monsieurtanuki]

I've just run pana on the latest version of the code, and got a score of 90/100. The current score on pub.dev is 70/110:

• it looks like with the recent changes we earned 20 points :)
• the probable pub.dev score for the next version is 90/110
• in pana, the 10 points we miss is because we don't support null-safety (which is only in beta). I think we should play it safe and wait until dart 2.12 is stable - then we'll be able to migrate to null-safety.
• why 100 in pana and 110 in pub.dev ? It looks like in pana they don't compute the 10-point score for the comments. So far we have dart comments only on 2.6%, and the threshold is 20%...

0/10 points: 20% or more of the public API has dartdoc comment
41 out of 1598 API elements (2.6 %) have documentation comments.
Providing good documentation for libraries, classes, functions, and other API elements improves code readability and helps developers find and use your API. Document at least 20% of the public API elements.

[MohamedFBoussaid]

@monsieurtanuki a great job was done there thanks, and it is a huge improvement for the score.
And for the null safety, I have tried to make the package null safety, but it is too risky like walking on a minefield. When the time comes to move to null safety we should create a internal version that we can test first (I don't if it is possible with pub.dv to have a version with restricted access)

[monsieurtanuki]

I agree with you @MohamedFBoussaid, no rush.
As for the tests of the null-safety version, I think it will be a good opportunity for an improvement of the unit test code on a broader range (if needed). We'll see then.

[M123-dev]

I just updated the packages to the newest version #98.
If I haven't missed any, all the packages we use already support null safety. That means we are ready for the first test.🎉

[monsieurtanuki]

Version 1.3.0 - still some work to be done for a score of 130/130:

170 out of 1859 API elements (9.1 %) have documentation comments.
Providing good documentation for libraries, classes, functions, and other API elements improves code readability and helps developers find and use your API. Document at least 20% of the public API elements.

[monsieurtanuki]

I don't know how pub.dev counts the API elements; nevertheless I've roughly counted the number of lines of each file in the Android Studio "structure" tab and found about 1700 - compared to the 1859 indicated by pub.dev.

In order to exceed the 20% threshold, I'm going to be lazy and just comment stupidly the languages (186) and the nutrients (170): we should reach around 28% of comments, and we'll get the 130/130 score. Hopefully...

[M123-dev]

We did it, we reached the 130/130 score 🥇

Thanks @monsieurtanuki for your provided mass documentation, we reached a stunning 46.7 % coverage

[monsieurtanuki]

Wonderful! Well, it took about 8 PRs to go from 80 to 130 points. I guess now we have enough code stability to stay at 130.