model: Replace attrs-strict with stricter validation
ClosedPublic
Actions

Authored by vlorentz on Sep 24 2021, 2:26 PM.

Details

Reviewers

douardda

Group Reviewers

Reviewers

Commits

rDMOD734b0812c165: model: Replace attrs-strict with stricter validation

Summary

This reimplements attrs_strict.type_validator(), using type equality
instead of isinstance.

This makes my checksum validation script (that mostly just instantiates
model objects, computes a checksum, then discard) run twice as fast.

I'm reusing the exception from attrs_strict for compatibility with
existing code that might rely on it. (Though I don't think there's any).

If you're fine with this diff in principle, I'll add tests for this
new type checker.

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

vlorentz created this revision.Sep 24 2021, 2:26 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptSep 24 2021, 2:26 PM

Build is green

Patch application report for D6341 (id=23056)

Rebasing onto e30eb7d291...

Current branch diff-target is up to date.

Changes applied before test

commit 9318d3b15096e349c43bf0650e4641eae0a0e06e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Sep 24 14:24:36 2021 +0200

    model: Replace attrs-strict with stricter validation
    
    This reimplements attrs_strict.type_validator(), using type equality
    instead of isinstance.
    
    This makes my checksum validation script (that mostly just instantiates
    model objects, computes a checksum, then discard) run twice as fast.

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

Harbormaster completed remote builds in B23983: Diff 23056.Sep 24 2021, 2:28 PM

vlorentz requested review of this revision.Sep 24 2021, 2:28 PM

olasd added a subscriber: olasd.Sep 24 2021, 5:38 PM

olasd added inline comments.

swh/model/model.py
136

Looks fine to me, but it needs some extensive tests indeed.

swh/model/model.py
95	we kind of need it Out of curiosity, why?
101	Why `origin not in (Union,)` instead of `origin is not Union` ?
121	Might be useful to ad a comment on why there is nothing related to standard dict, maybe.

vlorentz added inline comments.Sep 27 2021, 10:28 AM

swh/model/model.py
95	directory entry permissions. I guess we could add a converter from DentryPerms to int though.
101	I expected I would need more when writing it, but it turns out I don't

add extensive testing
remove special-casing of IntEnum and add converter for Directory.perms
replace origin not in (Union,)
fix a typo (raise False)
add comment explaining why we don't check for dict and list

Build is green

Patch application report for D6341 (id=23119)

Rebasing onto e30eb7d291...

Current branch diff-target is up to date.

Changes applied before test

commit 734b0812c165892039b2a9bc74e51a29e21fb0f9
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Fri Sep 24 14:24:36 2021 +0200

    model: Replace attrs-strict with stricter validation
    
    This reimplements attrs_strict.type_validator(), using type equality
    instead of isinstance.
    
    This makes my checksum validation script (that mostly just instantiates
    model objects, computes a checksum, then discard) run twice as fast.

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

Harbormaster completed remote builds in B24036: Diff 23119.Sep 28 2021, 11:55 AM

Test coverage looks fairly complete, thx

This revision is now accepted and ready to land.Sep 29 2021, 10:45 AM

Closed by commit rDMOD734b0812c165: model: Replace attrs-strict with stricter validation (authored by vlorentz). · Explain WhySep 29 2021, 3:52 PM

This revision was automatically updated to reflect the committed changes.

vlorentz added a commit: rDMOD734b0812c165: model: Replace attrs-strict with stricter validation.