Page MenuHomeSoftware Heritage

Refactor the model and simplify a bit origin.py
ClosedPublic

Authored by douardda on Mar 30 2021, 5:30 PM.

Details

Summary
  • kill the useless OriginIterator class,
  • remove commented thread locks
  • move all *Entry model classes in model.py,
  • kill the useless TreeEntry base class,
  • do not keep the archive instance as instance attribute of model objects, instead
  • implement "iterators" as explicit methods taking an ArchiveInterface object as argument,
  • refactor the code in provenance.py accordingly.

Note: we are far from a nice and clean model here, just trying to improve things step by step.

Depends on D5337

Diff Detail

Repository
rDPROV Provenance database
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Build is green

Patch application report for D5387 (id=19289)

Could not rebase; Attempt merge onto 4a5a99ea7d...

Updating 4a5a99e..9583d03
Fast-forward
 swh/provenance/archive.py                          |   6 +-
 swh/provenance/cli.py                              |   2 +-
 swh/provenance/model.py                            |  83 ++++++++++----
 swh/provenance/origin.py                           |  53 +++------
 swh/provenance/postgresql/archive.py               |   6 +-
 swh/provenance/provenance.py                       |  62 +++++++----
 swh/provenance/revision.py                         |  44 +-------
 swh/provenance/storage/archive.py                  |  10 +-
 swh/provenance/tests/conftest.py                   | 123 ++++++++++++++++++++-
 .../tests/data/synthetic_noroot_lower.txt          |  92 +++++++++++++++
 .../tests/data/synthetic_noroot_upper.txt          |  92 +++++++++++++++
 swh/provenance/tests/test_provenance_db.py         |  88 +++++++++++++--
 12 files changed, 520 insertions(+), 141 deletions(-)
 create mode 100644 swh/provenance/tests/data/synthetic_noroot_lower.txt
 create mode 100644 swh/provenance/tests/data/synthetic_noroot_upper.txt
Changes applied before test
commit 9583d0348f9013295f201944d63b69328fc87fd7
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Mar 30 14:49:38 2021 +0200

    Refactor the model
    
    - move all *Entry model classes in model.py,
    - kill the useless TreeEntry base class,
    - do not keep the archive instance as instance attribute of model
      objects, instead
    - implement "iterators" as explicit methods taking an ArchiveInterface
      object as argument,
    - refactor the code in provenance.py accordingly.

commit e97d2e2473efd39e08018914606438fd5e91b91d
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Mar 30 12:01:07 2021 +0200

    Simplify a bit origin.py
    
    - kill the useless OriginIterator class,
    - remove commented thread locks

commit 88c3910d2da58cc4167fb72b7ec530eec51cad8b
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Mar 25 15:01:35 2021 +0100

    Add a test for the (noroot, upper) case

commit 5d3f925e42530de891d94041046b2aec66d182c3
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed Mar 24 09:49:11 2021 +0100

    Add a test to compare the result of revision_add() with known results
    
    Use a 'synthetic' file to describe the expected behavior of the
    insertion of revisions in the provenance index.

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

test coverage of the code touched by this diff isn't great

swh/provenance/archive.py
20–24 ↗(On Diff #19289)

why?

test coverage of the code touched by this diff isn't great

I known, that's what I am trying to fight currently...

swh/provenance/archive.py
20–24 ↗(On Diff #19289)

why should I enforce a list here?

I known, that's what I am trying to fight currently...

ok, cool :)

why should I enforce a list here?

Because it's a footgun. We already decided to enforce List in swh-storage instead of Iterable because some functions in swh-storage accidentally expected it to be a sequence, and we didn't see it in tests because tests didn't check non-sequence iterables. And mypy is no help here either.

Build is green

Patch application report for D5387 (id=19834)

Could not rebase; Attempt merge onto 62617e5006...

Updating 62617e5..a23a33c
Fast-forward
 swh/provenance/archive.py                          |   6 +-
 swh/provenance/cli.py                              |   2 +-
 swh/provenance/model.py                            |  83 ++++++++++----
 swh/provenance/origin.py                           |  53 +++------
 swh/provenance/postgresql/archive.py               |   6 +-
 swh/provenance/provenance.py                       |  62 +++++++----
 swh/provenance/revision.py                         |  44 +-------
 swh/provenance/storage/archive.py                  |  10 +-
 swh/provenance/tests/conftest.py                   | 123 ++++++++++++++++++++-
 .../tests/data/synthetic_noroot_lower.txt          |  92 +++++++++++++++
 .../tests/data/synthetic_noroot_upper.txt          |  92 +++++++++++++++
 swh/provenance/tests/test_provenance_db.py         |  88 +++++++++++++--
 12 files changed, 520 insertions(+), 141 deletions(-)
 create mode 100644 swh/provenance/tests/data/synthetic_noroot_lower.txt
 create mode 100644 swh/provenance/tests/data/synthetic_noroot_upper.txt
Changes applied before test
commit a23a33c5a77da059f72835c0400c4384a04b7f64
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Mar 30 14:49:38 2021 +0200

    Refactor the model
    
    - move all *Entry model classes in model.py,
    - kill the useless TreeEntry base class,
    - do not keep the archive instance as instance attribute of model
      objects, instead
    - implement "iterators" as explicit methods taking an ArchiveInterface
      object as argument,
    - refactor the code in provenance.py accordingly.

commit 7eaeebb6091a13105ddee289fdcf9f70cb367c8e
Author: David Douard <david.douard@sdfa3.org>
Date:   Tue Mar 30 12:01:07 2021 +0200

    Simplify a bit origin.py
    
    - kill the useless OriginIterator class,
    - remove commented thread locks

commit 8853314af9817988a58450937c9d25097e66e741
Author: David Douard <david.douard@sdfa3.org>
Date:   Thu Mar 25 15:01:35 2021 +0100

    Add a test for the (noroot, upper) case

commit adbc99dd357dc452081df1965805756987b7e09b
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed Mar 24 09:49:11 2021 +0100

    Add a test to compare the result of revision_add() with known results
    
    Use a 'synthetic' file to describe the expected behavior of the
    insertion of revisions in the provenance index.

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

This revision was not accepted when it landed; it landed in state Needs Review.Mon, Apr 19, 4:56 PM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.