Page MenuHomeSoftware Heritage

swh.web.common.service: Update lookup_content_license.
ClosedPublic

Authored by s on Mar 17 2018, 9:51 PM.

Details

Summary

Modify lookup_content_license to accomodate change in the return value
of IndexerStorage's content_fossology_license_get method (See D301).

Now, lookup_content_license function returns a dictionary with the
following keys:

  • id: sha1 hash
  • facts: list of dictionaries containing license information reported by one or more tools.

Related T782

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Sounds good.

One minor nitpick in the description

Now, lookup_content_license function returns a dictionary with the following keys

Now reading this comment and the code could be confusing (we say it's a dict but the code extract data from list ;)
So clarifying, D301 introduces a dict generator (well, depending on the deployed configuration).
As you mentioned, each dict holds 1 key sha1 and 1 value which are facts, list of {tool + associated detected license for that tool on the sha1}

Oh, also, i believe the tests need some love:

make test

should give you some hints ;)

In D302#6153, @ardumont wrote:

Sounds good.

One minor nitpick in the description

Now, lookup_content_license function returns a dictionary with the following keys

Now reading this comment and the code could be confusing (we say it's a dict but the code extract data from list ;)
So clarifying, D301 introduces a dict generator (well, depending on the deployed configuration).
As you mentioned, each dict holds 1 key sha1 and 1 value which are facts, list of {tool + associated detected license for that tool on the sha1}

I'm slightly confused now. The content_fossology_license_get call in lookup_content_license yields a list of dictionaries and lookup_content_license returns a dictionary with keys id and facts, correct? If yes, is it incorrect to say "lookup_content_license function returns a dictionary"?

In D302#6155, @ardumont wrote:

Oh, also, i believe the tests need some love:

make test

should give you some hints ;)

Yes, I'll update the failing tests :)

  • swh.web.tests.common.test_service: Fix tests in ServiceTestCase
In D302#6155, @ardumont wrote:

Oh, also, i believe the tests need some love:
...

Done.

I'm slightly confused now.

Don't be ;)

The content_fossology_license_get call in lookup_content_license yields a list of dictionaries and lookup_content_license returns a dictionary with keys id and facts, correct?

Yes.

The content_fossology_license_get call in lookup_content_license yields a list of dictionaries

To give details, it either yield dictionaries (local indexer storage api call) or returns a list of dictionaries (remote indexer storage api call).
This depends on the swh-indexer's storage api client configuration set.
For example, in production for the webapp (here the indexer storage api client), it's a list since we are using remote indexer storage.

If yes, is it incorrect to say "lookup_content_license function returns a dictionary"?

No, it's not.
I did not say you were incorrect.

I was pointing out possible confusion for other readers than us, I tried to lift those (wrong choice apparently ;)

So, i should have either refrained from commenting or commenting directly on the code instead, lesson learned!

Anyway, sorry for the noise.

This sounds good to go.

This revision is now accepted and ready to land.Mar 20 2018, 10:53 AM

Tested locally and all fine. Nice job!

In D302#6173, @ardumont wrote:

Tested locally and all fine. Nice job!

Can I merge the changes into master branch and push it?

This revision was automatically updated to reflect the committed changes.