Page MenuHomeSoftware Heritage

Add type hints to resolver modules
ClosedPublic

Authored by jayeshv on Sep 13 2022, 2:32 PM.

Details

Summary

Related to T4261

Diff Detail

Repository
rDGQL GraphQL API
Branch
types
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 31506
Build 49287: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 49286: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D8462 (id=30490)

Rebasing onto 7e4eacb71f...

Current branch diff-target is up to date.
Changes applied before test
commit 813e9243b611052f4f5a9d774248b1a08e9fa750
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Tue Sep 13 12:03:14 2022 +0200

    Add type hints to resolver modules
    
    Related to T4261

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

anlambert added a subscriber: anlambert.

Looks good to me, just one missing return type to add.

Reading you code more deeply, I think it would be better to get rid of the resolvers_factory module
as it adds a not really needed indirection and makes code hard to follow.

Returning the proper node object directly in the *_resolver functions seems simpler to me and
you can set a local mapping based on object type in them when returning an Union.

swh/graphql/resolvers/resolvers.py
287

-> rs.revision.LogRevisionConnection

This revision is now accepted and ready to land.Sep 13 2022, 3:49 PM

add missing return; from review comments

Build is green

Patch application report for D8462 (id=30499)

Rebasing onto f2f222267f...

First, rewinding head to replay your work on top of it...
Applying: Add type hints to resolver modules
Changes applied before test
commit 4011c0393be9cdbf3b29fb7d3a2f102382830089
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Tue Sep 13 12:03:14 2022 +0200

    Add type hints to resolver modules
    
    Related to T4261

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

Looks good to me, just one missing return type to add.

Reading you code more deeply, I think it would be better to get rid of the resolvers_factory module
as it adds a not really needed indirection and makes code hard to follow.

Returning the proper node object directly in the *_resolver functions seems simpler to me and
you can set a local mapping based on object type in them when returning an Union.

Thanks. I have a diff planned for resolver factory. I will replace that with a proper factory method.

The idea with the factory was to help a user get the resolver class without reading any other code.
But it got a bit complex later, I will simplify that.

Build is green

Patch application report for D8462 (id=30503)

Rebasing onto f2f222267f...

Current branch diff-target is up to date.
Changes applied before test
commit f5ef0a1f8b4ca0335a0456f043c534cf5ffeac90
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Tue Sep 13 12:03:14 2022 +0200

    Add type hints to resolver modules
    
    Related to T4261

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

This revision was automatically updated to reflect the committed changes.