Page MenuHomeSoftware Heritage

swh.indexer.storage: Refactor fossology license get
ClosedPublic

Authored by s on Mar 16 2018, 4:46 AM.

Details

Summary

The content_fossology_license_get method in IndexerStorage now
returns a list of dictionaries whose keys are id and facts.

The facts key's value is a list of dictionaries. Each dictionary in
this list contains license information found for id by tool FOO.

Related T782

Diff Detail

Repository
rDCIDX Metadata indexer
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont added a subscriber: ardumont.
ardumont added inline comments.
swh/indexer/storage/__init__.py
330

Seems complicated. Why don't you just inline the _generate_dict call?

I think you tried your best to keep the format so that nothing breaks in the webapp ;)

But, as explained in T782#18456, i was more inclined to either return:

  1. directly the dict d (if possible at all, depending on our @db_transation* decorators)
  2. a list of dict with key the content id and value the facts (licenses + tool that discovered it).

So, in the current state, instead of the proposed statement, a simpler adaptation to match solution 2. would be :

for _id, facts in d.items():
    yield {_id: facts}

That is still a list, so nothing should break down hill (webapp, except probably for the id which won't be converted in hex, not a breaking per say).

We need to also adjust the webapp endpoint (as mentioned in the link above and repeated here for convenience).

544

As explained in previous comment, i'd simply remove this.

This revision now requires changes to proceed.Mar 16 2018, 10:09 AM

Change structure of yield value for content_fossology_license_get

s marked 2 inline comments as done.Mar 17 2018, 9:55 PM
s added inline comments.
swh/indexer/storage/__init__.py
330

We need to also adjust the webapp endpoint (as mentioned in the link above and repeated here for convenience).

I've opened D302; contains changes to the webapp endpoint.

Thanks!

Sounds good \m/

Remains to adapt the tests accordingly ;)

Remains to adapt the tests accordingly ;)

I forgot to mention that:

make test

should give you hints about what to adapt.

Cheers,

s marked an inline comment as done.
  • swh: Fix test for db_to_fossology_license.
  • swh: Fix tests in CommonTestStorage.
In D301#6156, @ardumont wrote:

Remains to adapt the tests accordingly ;)

...

Done.

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

Tested locally and all fine. Nice job!

In D301#6174, @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.

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

I'm in charge of this, thanks for asking though. And thanks for your contributions.
This is now referenced in the repository.

Cheers,