Paths

Table of Contentst

Differential D7342

Type annotations in metadata mappings
Needs RevisionPublic
Actions

Authored by lucifer4j on Mar 13 2022, 9:42 AM.

Details

Reviewers

vlorentz

Group Reviewers

Reviewers

Maniphest Tasks

T2259: Add type annotations to metadata mappings

Required Signatures

L3 Software Heritage Contributor License Agreement, version 1.0

Summary

Add type annotations to metadata mappings (T2259)

This patch adds type annotations to metadata mappings so that the mypy
type checker can detect bugs in code. While working on the path, I also
encountered these issues:

mypy raises error on detecting for implicit None returns, so I had to add return None at many places.

At a few places, @property s' are accessed using cls. mypy rightfully raises an error, so I had to add ignore to silence it for now. It appears Python 3.9+ supports class level properties so the ignores could be replaced by the proper handling when the minimum python version is raised.

Closes T2259.

Diff Detail

Repository

rDCIDX Metadata indexer

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 27424
Build 42914: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 42913: arc lint + arc unit

Event Timeline

lucifer4j created this revision.Mar 13 2022, 9:42 AM

Herald added a required legal document: L3 Software Heritage Contributor License Agreement, version 1.0. · View Herald TranscriptMar 13 2022, 9:42 AM

Build has FAILED

Patch application report for D7342 (id=26539)

Rebasing onto 9e89b83be4...

Current branch diff-target is up to date.

Changes applied before test

commit f60c67aa77c039db8f5399c7a79577b1dd658809
Author: Kartik Ohri <kartikohri13@gmail.com>
Date:   Sun Mar 13 14:09:23 2022 +0530

    Type annotations in metadata mappings
    
    Summary: Add type annotations to files in swh/indexer/metadata_dictionary/ (T2259)
    
    This patch adds type annotations to metadata mappings so that the mypy
    type checker can detect bugs in code. While working on the path, I also
    encountered these issues:
    
    1) TypedDict does not support using variables in defining or assigning
    keys. At a lot of places, the code uses SCHEMA_URI to construct the
    keys. mypy raises errors at these places. There is a tradeoff between
    replacing SCHEMA_URI with its literal value everywhere and using
    type: ignore hints.
    
    2) mypy raises error on detecting for implicit None returns, so I had
    to add return None at many places.
    
    3) At a few places, @property s' are accessed using `cls`. mypy
    rightfully raises an error, so I had to add ignore to silence it for
    now. It appears Python 3.9+ supports class level properties so the
    ignores could be replaced by the proper handling when the minimum
    python version is raised.
    
    Closes T2259.
    
    Reviewers: vlorentz

Link to build: https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/197/
See console output for more information: https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/197/console

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 13 2022, 9:43 AM

Harbormaster failed remote builds in B27421: Diff 26539!

Fix mypy issues

Build has FAILED

Patch application report for D7342 (id=26540)

Rebasing onto 9e89b83be4...

Current branch diff-target is up to date.

Changes applied before test

commit 518ac10085e4109572a9108726286c6a18441b69
Author: Kartik Ohri <kartikohri13@gmail.com>
Date:   Sun Mar 13 15:51:53 2022 +0530

    Fix mypy issues

commit f60c67aa77c039db8f5399c7a79577b1dd658809
Author: Kartik Ohri <kartikohri13@gmail.com>
Date:   Sun Mar 13 14:09:23 2022 +0530

    Type annotations in metadata mappings
    
    Summary: Add type annotations to files in swh/indexer/metadata_dictionary/ (T2259)
    
    This patch adds type annotations to metadata mappings so that the mypy
    type checker can detect bugs in code. While working on the path, I also
    encountered these issues:
    
    1) TypedDict does not support using variables in defining or assigning
    keys. At a lot of places, the code uses SCHEMA_URI to construct the
    keys. mypy raises errors at these places. There is a tradeoff between
    replacing SCHEMA_URI with its literal value everywhere and using
    type: ignore hints.
    
    2) mypy raises error on detecting for implicit None returns, so I had
    to add return None at many places.
    
    3) At a few places, @property s' are accessed using `cls`. mypy
    rightfully raises an error, so I had to add ignore to silence it for
    now. It appears Python 3.9+ supports class level properties so the
    ignores could be replaced by the proper handling when the minimum
    python version is raised.
    
    Closes T2259.
    
    Reviewers: vlorentz

Link to build: https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/198/
See console output for more information: https://jenkins.softwareheritage.org/job/DCIDX/job/tests-on-diff/198/console

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 13 2022, 11:26 AM

Harbormaster failed remote builds in B27422: Diff 26540!

Remove TypedDict's as per chat

Build is green

Patch application report for D7342 (id=26543)

Rebasing onto 9e89b83be4...

Current branch diff-target is up to date.

Changes applied before test

commit 601b6a0def92f7324cdb2ca12a20ba4f1504b62f
Author: Kartik Ohri <kartikohri13@gmail.com>
Date:   Mon Mar 14 15:29:48 2022 +0530

    Remove TypedDict's as per chat

commit 518ac10085e4109572a9108726286c6a18441b69
Author: Kartik Ohri <kartikohri13@gmail.com>
Date:   Sun Mar 13 15:51:53 2022 +0530

    Fix mypy issues

commit f60c67aa77c039db8f5399c7a79577b1dd658809
Author: Kartik Ohri <kartikohri13@gmail.com>
Date:   Sun Mar 13 14:09:23 2022 +0530

    Type annotations in metadata mappings
    
    Summary: Add type annotations to files in swh/indexer/metadata_dictionary/ (T2259)
    
    This patch adds type annotations to metadata mappings so that the mypy
    type checker can detect bugs in code. While working on the path, I also
    encountered these issues:
    
    1) TypedDict does not support using variables in defining or assigning
    keys. At a lot of places, the code uses SCHEMA_URI to construct the
    keys. mypy raises errors at these places. There is a tradeoff between
    replacing SCHEMA_URI with its literal value everywhere and using
    type: ignore hints.
    
    2) mypy raises error on detecting for implicit None returns, so I had
    to add return None at many places.
    
    3) At a few places, @property s' are accessed using `cls`. mypy
    rightfully raises an error, so I had to add ignore to silence it for
    now. It appears Python 3.9+ supports class level properties so the
    ignores could be replaced by the proper handling when the minimum
    python version is raised.
    
    Closes T2259.
    
    Reviewers: vlorentz

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

Harbormaster completed remote builds in B27424: Diff 26543.Mar 14 2022, 11:04 AM

lucifer4j requested review of this revision.Mar 14 2022, 11:04 AM

lucifer4j edited the summary of this revision. (Show Details)Mar 14 2022, 11:06 AM

lucifer4j edited the summary of this revision. (Show Details)

lucifer4j added inline comments.Mar 14 2022, 11:09 AM

swh/indexer/metadata_dictionary/base.py
110	Need to add # type: ignore because property is accessed using `cls`.

Hi,

Thanks for this, it looks good.

One small change, though: instead of repeating Dict[str, Any] (and Dict[str, str] and others) everywhere, you could create a specific type alias for dict that represent JSON-LD documents, eg. JsonldDict = Dict[str, Any], and use it in signatures

This revision now requires changes to proceed.Mar 14 2022, 11:46 AM