maven: Simplify definition of the 'version_artifact' dict
ClosedPublic
Actions

Authored by vlorentz on Dec 7 2021, 1:34 PM.

Details

Reviewers

borisbaldassari
ardumont

Group Reviewers

Reviewers

Commits

rDLDBASE5da115b6e5bf: maven: Simplify definition of the 'version_artifact' dict

Summary

We don't need it to be ordered; and '.keys()' is redundant.

Diff Detail

Repository

rDLDBASE Generic VCS/Package Loader

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.Dec 7 2021, 1:34 PM

Build is green

Patch application report for D6766 (id=24560)

Could not rebase; Attempt merge onto 79b1075e1d...

Updating 79b1075..5da115b
Fast-forward
 requirements.txt                   |  1 +
 swh/loader/package/maven/loader.py | 93 ++++++++++++++++----------------------
 2 files changed, 39 insertions(+), 55 deletions(-)

Changes applied before test

commit 5da115b6e5bf48e3829c830c9f164bd07ed14509
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Dec 7 13:34:29 2021 +0100

    maven: Simplify definition of the 'version_artifact' dict
    
    We don't need it to be ordered; and '.keys()' is redundant.

commit ccf71383c61d642f717256a5ed55539073fd0477
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Dec 7 13:32:26 2021 +0100

    maven: Simplify build_extrinsic_directory_metadata.

commit a76ab28824a2c203b8cc5f9ff70cecf922770662
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Dec 7 11:54:33 2021 +0100

    maven: Add typing to the artifacts dict

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

Harbormaster completed remote builds in B25432: Diff 24560.Dec 7 2021, 1:36 PM

vlorentz requested review of this revision.Dec 7 2021, 1:36 PM

ardumont added a subscriber: ardumont.Dec 7 2021, 3:25 PM

ardumont added inline comments.

swh/loader/package/maven/loader.py
139	ah! I'm pretty sure, the ordered version you dropped to be used here to enforce and take the last entry here... But the comment line 129 renders things confusing now. I'm not sure there is really order enforcement lister side...

ardumont added inline comments.Dec 7 2021, 3:27 PM

swh/loader/package/maven/loader.py
139	That is my previous comment with this change ^ (keeping the order that you dropped ...)

vlorentz added inline comments.Dec 7 2021, 3:32 PM

swh/loader/package/maven/loader.py
139	I don't understand what you are saying. I didn't functionaly change the behavior, did I?

ardumont added inline comments.Dec 7 2021, 3:52 PM

swh/loader/package/maven/loader.py
139	No, you did not change anything. I'm saying that the ordered version was initially there to ensure we used correctly the last version from the inputs of the loader. And it was not done correctly. Hence why you are able to drop it in that diff. But, now, i don't recall having seen anything in the lister that do indeed enforce order like the comment in line 129 says. So i'm not sure that your change (iso functional) and the initial code do the right thing about the last version of the release... Now, if you read back what i have initially said with my code change (and without yours), you might better understand my initial comment?

vlorentz added inline comments.Dec 7 2021, 3:57 PM

swh/loader/package/maven/loader.py
139	it's probably fine in the lister, by virtue of preserving the order from the index? idk, it's not tested anyway

ardumont added inline comments.Dec 7 2021, 4:19 PM

swh/loader/package/maven/loader.py
139	it's probably fine in the lister, by virtue of preserving the order from the index? Yes, but is the order preserved when the loader receive stuff? Oh well, let's keep your diff and this in our mind then. And at some point in the future, we need to make sure we have the default version. Whatever that is in maven... idk, it's not tested anyway yes, i forgot to say it's not tested. But it should!