Unused methods were removed and type annotations fixed.
Details
Details
Diff Detail
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
Comment Actions
Build is green
Patch application report for D5862 (id=20985)
Rebasing onto 206399eb8a...
Current branch diff-target is up to date.
Changes applied before test
commit 8fcffa112104aeca2faff5cc669cfc37e2b27b43
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Mon Jun 14 13:59:30 2021 +0200
Rework ArchiveInterface
Unused methods were removed and time annotations fixed.See https://jenkins.softwareheritage.org/job/DPROV/job/tests-on-diff/129/ for more details.
Comment Actions
Build is green
Patch application report for D5862 (id=20986)
Rebasing onto 206399eb8a...
Current branch diff-target is up to date.
Changes applied before test
commit a30a6794422d281f91b656f38b45368ec9d4e47c
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Mon Jun 14 13:59:30 2021 +0200
Rework ArchiveInterface
Unused methods were removed and type annotations fixed.See https://jenkins.softwareheritage.org/job/DPROV/job/tests-on-diff/130/ for more details.
| swh/provenance/model.py | ||
|---|---|---|
| 69 | This date conversion chunk of code should not be part of this revision (nor this Diff). | |
| swh/provenance/model.py | ||
|---|---|---|
| 69 | Why not? If I don't do the conversion mypy will complain that the types don't match | |
| swh/provenance/model.py | ||
|---|---|---|
| 69 | First, it looks to me much more than just a "please mypy" kind of fix. Removing dead code and fixing stuff should not be in the same revision. Then, if you need to fix this date handling first, fine, put this fix in a dedicated revision (and a dedicated Diff), then make your code cleanup on the top of that. If you want to fix mypy, the first thing you need is to add type annotations to the ArchiveInterface (which may be part of a "rework ArchiveInterface" revision/diff). But I insist, this fix is by no mean a "make mypy happy" kind of fix. The code (before this conversion) is just broken. So it needs a proper fix. With tests. So IMHO this needs to be split in 2 parts:
| |
Comment Actions
Build is green
Patch application report for D5862 (id=21060)
Rebasing onto c9d1369ba1...
Current branch diff-target is up to date.
Changes applied before test
commit 8aa8e5ed543e1ca20f867f0f7886e7e30502d5e8
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Mon Jun 14 13:59:30 2021 +0200
Rework ArchiveInterface
Unused methods were removed and type annotations fixed.See https://jenkins.softwareheritage.org/job/DPROV/job/tests-on-diff/141/ for more details.
| swh/provenance/archive.py | ||
|---|---|---|
| 19 | It should also be the right time to also document the interface. | |
Comment Actions
Build is green
Patch application report for D5862 (id=21085)
Could not rebase; Attempt merge onto c9d1369ba1...
Updating c9d1369..f309f67 Fast-forward swh/provenance/archive.py | 42 +++++++++++++++------ swh/provenance/model.py | 72 +++++++++++++----------------------- swh/provenance/postgresql/archive.py | 67 ++++++++++++++++++--------------- swh/provenance/storage/archive.py | 72 +++++++++++++++++++----------------- 4 files changed, 131 insertions(+), 122 deletions(-)
Changes applied before test
commit f309f67a524cc29d12ae4cfb65c482f5aa6aaac9
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Thu Jun 17 13:51:54 2021 +0200
Rework ArchiveInterface
Unused methods were removed and type annotations fixed.
Other methos in OriginEntry and RevisionEntry were updated accordingly
commit 12edf4a3b7992575f5f24cd8a648c803dbda142a
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Thu Jun 17 13:48:24 2021 +0200
Fix bugs when retrieven parents in RevisionEntrySee https://jenkins.softwareheritage.org/job/DPROV/job/tests-on-diff/145/ for more details.
Comment Actions
Build is green
Patch application report for D5862 (id=21089)
Could not rebase; Attempt merge onto c9d1369ba1...
Updating c9d1369..a12961f Fast-forward swh/provenance/archive.py | 42 +++++++++++++++------ swh/provenance/model.py | 72 +++++++++++++----------------------- swh/provenance/postgresql/archive.py | 67 ++++++++++++++++++--------------- swh/provenance/storage/archive.py | 72 +++++++++++++++++++----------------- 4 files changed, 131 insertions(+), 122 deletions(-)
Changes applied before test
commit a12961f7536eb2e25c81b4f106c028dc6c655b29
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Thu Jun 17 13:51:54 2021 +0200
Rework ArchiveInterface
Unused methods were removed and type annotations fixed.
Other methos in OriginEntry and RevisionEntry were updated accordingly
commit 12edf4a3b7992575f5f24cd8a648c803dbda142a
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Thu Jun 17 13:48:24 2021 +0200
Fix bugs when retrieven parents in RevisionEntrySee https://jenkins.softwareheritage.org/job/DPROV/job/tests-on-diff/149/ for more details.
Comment Actions
Thanks for the split. Almost there, but a few nitpicks/comments remains:
- typo in the commit message ("s/methos/mothods/")
- prefer the active form in the commit message (describe what the commit does, and why), i.e. "remove unused methods" instead of "unused methods were removed", etc.
- the code duplication in both implementations of snapshot_get_heads() should be avoided (it's ok for 2 liners to be duplicated, but not for a 25 lines function); maybe implement its "core" as a function and use it from these methods?
Comment Actions
OK for the first two items, but I don't agree on the third one. The idea is to replace one of them by a direct SQL query in the near future so reworking this will be useless. I just didn't implement the query because I needed to move forward with the other stuff and we know this implementation actually works (it was manually tested). Also, I believe that someone with better SQL knowledge will do a better job than me in implementing this query.
Comment Actions
Build is green
Patch application report for D5862 (id=21131)
Could not rebase; Attempt merge onto 8ff1ab5860...
Updating 8ff1ab5..969e335 Fast-forward requirements-swh.txt | 2 +- swh/provenance/archive.py | 42 +++++++++++++++------ swh/provenance/model.py | 48 +++++------------------- swh/provenance/postgresql/archive.py | 67 ++++++++++++++++++--------------- swh/provenance/storage/archive.py | 72 +++++++++++++++++++----------------- 5 files changed, 117 insertions(+), 114 deletions(-)
Changes applied before test
commit 969e3359123b781ae1101e2fb62b9934b0988478
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Thu Jun 17 13:51:54 2021 +0200
Rework ArchiveInterface
Remove Unused methods and fix type annotations. Update Other methods in
OriginEntry and RevisionEntry accordingly.
commit d28d3eed7049e36c5841405021fd0a0e78c11cb7
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Thu Jun 17 13:48:24 2021 +0200
Fix bugs when retrieving parents in RevisionEntry
Convert `Revision.date` from` TimestampWithTimezone` to `datetime` as expected by` RevisionEntry`.
Create a list with the iterator returned by `ArchiveInterface.revision_get ()` before comparison.See https://jenkins.softwareheritage.org/job/DPROV/job/tests-on-diff/162/ for more details.
Comment Actions
Ok fair enough; just add a comment in there to explain that;
and we know this implementation actually works (it was manually tested).
well, you know it worked a some point, but without tests, nobody can be sure of this (I know I am rambling, I know...)
Also, I believe that someone with better SQL knowledge will do a better job than me in implementing this query.
don't hesitate to ask for help to any "crew member" :-)
Comment Actions
but please add a comment in the ArchivePostgreSQL's version of snapshot_get_heads explaining why it's (for now) a duplication of the other implementation, thanks
Comment Actions
Build is green
Patch application report for D5862 (id=21139)
Could not rebase; Attempt merge onto 8ff1ab5860...
Updating 8ff1ab5..01bbb0c Fast-forward requirements-swh.txt | 2 +- swh/provenance/archive.py | 42 +++++++++++++++------ swh/provenance/model.py | 48 +++++------------------- swh/provenance/postgresql/archive.py | 70 ++++++++++++++++++++--------------- swh/provenance/storage/archive.py | 72 +++++++++++++++++++----------------- 5 files changed, 120 insertions(+), 114 deletions(-)
Changes applied before test
commit 01bbb0ce850a5cfafaa1837fa346a19b2a851fc9
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Thu Jun 17 13:51:54 2021 +0200
Rework ArchiveInterface
Remove Unused methods and fix type annotations. Update Other methods in
OriginEntry and RevisionEntry accordingly.
commit cd48f9d71efeaf73cfcc24f397d22589a5f7987b
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Thu Jun 17 13:48:24 2021 +0200
Fix bugs when retrieving parents in RevisionEntry
Convert `Revision.date` from` TimestampWithTimezone` to `datetime` as expected by` RevisionEntry`.
Create a list with the iterator returned by `ArchiveInterface.revision_get()` before comparison.See https://jenkins.softwareheritage.org/job/DPROV/job/tests-on-diff/167/ for more details.
Comment Actions
Build is green
Patch application report for D5862 (id=21171)
Could not rebase; Attempt merge onto 011645221c...
Updating 0116452..f354b65 Fast-forward requirements-swh.txt | 2 +- swh/provenance/archive.py | 42 +++++++++++++++------ swh/provenance/model.py | 48 +++++------------------- swh/provenance/postgresql/archive.py | 70 ++++++++++++++++++++--------------- swh/provenance/storage/archive.py | 72 +++++++++++++++++++----------------- 5 files changed, 120 insertions(+), 114 deletions(-)
Changes applied before test
commit f354b65e52ed78b3a637b2632e1b74bb920669be
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Thu Jun 17 13:51:54 2021 +0200
Rework ArchiveInterface
Remove Unused methods and fix type annotations. Update Other methods in
OriginEntry and RevisionEntry accordingly.
commit e6f39d0244b10b49942b0ab93d4628828e343642
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date: Thu Jun 17 13:48:24 2021 +0200
Fix bugs when retrieving parents in RevisionEntry
Convert `Revision.date` from` TimestampWithTimezone` to `datetime` as expected by` RevisionEntry`.
Create a list with the iterator returned by `ArchiveInterface.revision_get()` before comparison.See https://jenkins.softwareheritage.org/job/DPROV/job/tests-on-diff/177/ for more details.