Page MenuHomeSoftware Heritage

Add revision log field in the revision type
ClosedPublic

Authored by jayeshv on Aug 24 2022, 5:27 PM.

Details

Summary

Storage is returning a list of dicts for the revision log
Add a from_dict converter to create the model objects from these dicts
This is not efficient and adds an extra iteration. Has to be
fixed in the storage in the future.

Related to T4366

Diff Detail

Repository
rDGQL GraphQL API
Branch
revision-log
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 31043
Build 48566: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 48565: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D8310 (id=29993)

Rebasing onto 6fb1913ff9...

Current branch diff-target is up to date.
Changes applied before test
commit e21947ad862f0a75edbff1c87607d85e01a61fe9
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Wed Aug 24 17:04:56 2022 +0200

    Add revision log field in the revision type
    
    Storage is returning a list of dicts for the revision log
    Add a from_dict converter to create the model objects from these dicts
    This is not efficient and adds an extra iteration. Has to be
    fixed in the storage in the future.
    
    Related to T4366

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/122/ for more details.

Remove unnecessary comment

Build is green

Patch application report for D8310 (id=29994)

Rebasing onto 6fb1913ff9...

Current branch diff-target is up to date.
Changes applied before test
commit 12003a050365d3570b4e3d8ba9b0bf34810b0a1d
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Wed Aug 24 17:04:56 2022 +0200

    Add revision log field in the revision type
    
    Storage is returning a list of dicts for the revision log
    Add a from_dict converter to create the model objects from these dicts
    This is not efficient and adds an extra iteration. Has to be
    fixed in the storage in the future.
    
    Related to T4366

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/123/ for more details.

jayeshv retitled this revision from Add revision log field in the revision type to [WIP]Add revision log field in the revision type.Aug 25 2022, 11:45 AM
jayeshv edited the summary of this revision. (Show Details)
vlorentz retitled this revision from [WIP]Add revision log field in the revision type to [WIP] Add revision log field in the revision type.Aug 25 2022, 11:54 AM

Build is green

Patch application report for D8310 (id=30030)

Rebasing onto 40c0debad7...

Current branch diff-target is up to date.
Changes applied before test
commit e3fa1bbb5cdbb468a56b11ca408b0d4b78f95a90
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Wed Aug 24 17:04:56 2022 +0200

    Add revision log field in the revision type
    
    Storage is returning a list of dicts for the revision log.
    Add a from_dict converter to create the model objects from these dicts
    This is not efficient and adds an extra iteration. Has to be
    fixed in the storage in the future.
    
    Related to T4366

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/129/ for more details.

jayeshv retitled this revision from [WIP] Add revision log field in the revision type to Add revision log field in the revision type.Aug 25 2022, 3:18 PM
jayeshv edited the summary of this revision. (Show Details)
ardumont added a subscriber: ardumont.

lgtm

swh/graphql/resolvers/revision.py
105
swh/graphql/tests/data.py
87 ↗(On Diff #30030)

is it necessary to hard-code that id here?
If the revision object does not declare it, it will be recomputed once it's accessed, it might be better that way (same goes for other dag model object).

This revision is now accepted and ready to land.Aug 26 2022, 2:23 PM
swh/graphql/tests/data.py
87 ↗(On Diff #30030)

If I let that compute the SWHID, how do I use the same object while testing?
This revision is used in both "test_get_revision_log" and "test_get_revision_parents" functions below.

swh/graphql/tests/data.py
87 ↗(On Diff #30030)

You can have the objects be declared at the module level and then refer to them both in function (and/or fixture) and in test.

swh/graphql/tests/functional/test_revision.py
133 ↗(On Diff #30030)

i guess? (it's probably missing the hash to hex yadi yada though)

Build is green

Patch application report for D8310 (id=30057)

Rebasing onto 45582e79ee...

Current branch diff-target is up to date.
Changes applied before test
commit a31419d74c4562f3f9d4dad02a7b8ad1590aa97b
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Wed Aug 24 17:04:56 2022 +0200

    Add revision log field in the revision type
    
    Storage is returning a list of dicts for the revision log.
    Add a from_dict converter to create the model objects from these dicts
    This is not efficient and adds an extra iteration. Has to be
    fixed in the storage in the future.
    
    Related to T4366

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/132/ for more details.

anlambert added inline comments.
swh/graphql/tests/data.py
87 ↗(On Diff #30030)

I agree with ardumont, hard-coding SWHIDs could be avoided here.

You should wrap that revision in a fixture (without declaring the id as it is automatically computed when accessing the field)
and pass it as parameter of the tests that need it.

swh/graphql/tests/data.py
87 ↗(On Diff #30030)

Got it, Thanks. I will make another diff for those change. This I will land

87 ↗(On Diff #30030)

Thanks, I will do that