Page MenuHomeSoftware Heritage

type_validator: Re-allow subclasses
ClosedPublic

Authored by vlorentz on Oct 1 2021, 2:20 PM.

Details

Summary

The previous replaced attrs-strict's type validator with our own,
stricter and faster, validator.

However, the strictness can be a burden in other packages;
for example, swh-storage tests rely on it to insert dummy data that raises
exception when accessed, and it would be hard to do while using the exact
expected type.

This commit reverts the strict behavior, but keeps the performance
optimization, by always checking with type equality, but in case type
equality fails (which would raise an error before this commit), it gives
the value a 'second chance', by trying isinstance.

This means that, outside tests, isinstance should not be used at all,
or very rarely.

Diff Detail

Repository
rDMOD Data model
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Build has FAILED

Patch application report for D6387 (id=23234)

Rebasing onto 734b0812c1...

Current branch diff-target is up to date.
Changes applied before test
commit 36f709ffe627cb1fc988b217acf15c868122c2bf
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Oct 1 14:19:48 2021 +0200

    type_validator: Re-allow subclasses
    
    The previous replaced attrs-strict's type validator with our own,
    stricter and faster, validator.
    
    However, the strictness can be a burden in other packages;
    for example, swh-storage tests rely on it to insert dummy data that raises
    exception when accessed, and it would be hard to do while using the exact
    expected type.
    
    This commit reverts the strict behavior, but keeps the performance
    optimization, by always checking with type equality, but in case type
    equality fails (which would raise an error before this commit), it gives
    the value a 'second chance', by trying isinstance.
    
    This means that, outside tests, isinstance should not be used at all,
    or very rarely.

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

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 1 2021, 2:22 PM
Harbormaster failed remote builds in B24151: Diff 23234!

fix test; model objects all need to be final

Build is green

Patch application report for D6387 (id=23235)

Rebasing onto 734b0812c1...

Current branch diff-target is up to date.
Changes applied before test
commit 916627e1b504cbe696f71be5960ab34711cd03da
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Oct 1 14:19:48 2021 +0200

    type_validator: Re-allow subclasses
    
    The previous replaced attrs-strict's type validator with our own,
    stricter and faster, validator.
    
    However, the strictness can be a burden in other packages;
    for example, swh-storage tests rely on it to insert dummy data that raises
    exception when accessed, and it would be hard to do while using the exact
    expected type.
    
    This commit reverts the strict behavior, but keeps the performance
    optimization, by always checking with type equality, but in case type
    equality fails (which would raise an error before this commit), it gives
    the value a 'second chance', by trying isinstance.
    
    This means that, outside tests, isinstance should not be used at all,
    or very rarely.

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

This revision is now accepted and ready to land.Oct 4 2021, 4:04 PM
This revision was automatically updated to reflect the committed changes.