Page MenuHomeSoftware Heritage

identifiers.parse_swhid: Check the swhid qualifiers and fail if invalid
ClosedPublic

Authored by ardumont on Thu, Nov 12, 10:16 AM.

Details

Summary

Enfore checks on the qualifiers so invalid swhid are rejected.

Related to T2769
Depends on D4458

Test Plan

tox

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

ardumont created this revision.Thu, Nov 12, 10:16 AM

Build is green

Patch application report for D4461 (id=15816)

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

Updating 4e3fdc0..870fbc7
Fast-forward
 swh/model/identifiers.py            | 67 ++++++++++++++++++++++++++++------
 swh/model/tests/test_identifiers.py | 73 +++++++++++++++++++++----------------
 2 files changed, 97 insertions(+), 43 deletions(-)
Changes applied before test
commit 870fbc70dda134b3265f20b2054a788012ad4c13
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 12 10:08:45 2020 +0100

    identifiers.parse_swhid: Check the swhid qualifiers and fail if invalid
    
    Related to T2769

commit 87071750c81642933a5c936b7282d6da04dddd2d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Nov 10 18:21:01 2020 +0100

    model.identifiers: Improve error message by exposing the grammar
    
    Related to T2769

commit 47946aac0220fcfd55c7998908c71a6f402a5ddb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Nov 10 16:11:32 2020 +0100

    test: Migrate parse_swhid test cases to pytest
    
    Related to T2769

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

ardumont updated this revision to Diff 15819.Thu, Nov 12, 10:20 AM
  • Fix noisy characters in noqa comments
  • add comments to explicit what's wrong with swhid

Build is green

Patch application report for D4461 (id=15819)

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

Updating 4e3fdc0..49f3f5c
Fast-forward
 swh/model/identifiers.py            | 67 ++++++++++++++++++++++++++------
 swh/model/tests/test_identifiers.py | 77 ++++++++++++++++++++++---------------
 2 files changed, 101 insertions(+), 43 deletions(-)
Changes applied before test
commit 49f3f5c4253a27b14ecffe5755aa4895bfee85aa
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 12 10:08:45 2020 +0100

    identifiers.parse_swhid: Check the swhid qualifiers and fail if invalid
    
    Related to T2769

commit 87071750c81642933a5c936b7282d6da04dddd2d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Nov 10 18:21:01 2020 +0100

    model.identifiers: Improve error message by exposing the grammar
    
    Related to T2769

commit 47946aac0220fcfd55c7998908c71a6f402a5ddb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Nov 10 16:11:32 2020 +0100

    test: Migrate parse_swhid test cases to pytest
    
    Related to T2769

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

Build is green

Patch application report for D4461 (id=15821)

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

Updating 4e3fdc0..b924c07
Fast-forward
 swh/model/identifiers.py            | 71 ++++++++++++++++++++++++++++------
 swh/model/tests/test_identifiers.py | 77 ++++++++++++++++++++++---------------
 2 files changed, 105 insertions(+), 43 deletions(-)
Changes applied before test
commit b924c077d8681cbc82cbd809e9fbc4a9eb29c6f2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 12 10:08:45 2020 +0100

    identifiers.parse_swhid: Check the swhid qualifiers and fail if invalid
    
    Related to T2769

commit cb11afe5c7fae1301a26a9b500ec2c2932170e5c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Nov 10 18:21:01 2020 +0100

    model.identifiers: Improve error message by exposing the grammar
    
    Related to T2769

commit 47946aac0220fcfd55c7998908c71a6f402a5ddb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Nov 10 16:11:32 2020 +0100

    test: Migrate parse_swhid test cases to pytest
    
    Related to T2769

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

ardumont updated this revision to Diff 15824.Thu, Nov 12, 11:04 AM
  • Rebase
  • Adapt according to previous diff's review

Build is green

Patch application report for D4461 (id=15824)

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

Updating 4e3fdc0..20d7361
Fast-forward
 swh/model/identifiers.py            | 35 ++++++++++++-----
 swh/model/tests/test_identifiers.py | 77 ++++++++++++++++++++++---------------
 2 files changed, 72 insertions(+), 40 deletions(-)
Changes applied before test
commit 20d736195e8ec28918b90ec598a672fabedee702
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 12 10:08:45 2020 +0100

    identifiers.parse_swhid: Check the swhid qualifiers and fail if invalid
    
    Related to T2769

commit 8130a27e0261962bc5b36bddf914856ca24d15d2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Nov 10 18:21:01 2020 +0100

    model.identifiers: Improve error messages in case of invalid SWHIDs
    
    Related to T2769

commit 47946aac0220fcfd55c7998908c71a6f402a5ddb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Nov 10 16:11:32 2020 +0100

    test: Migrate parse_swhid test cases to pytest
    
    Related to T2769

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

vlorentz added inline comments.
swh/model/identifiers.py
849–866

You can remove the CONTEXT_QUALIFIERS check from the loop, then test after the loop if set(_metadata) - set(CONTEXT_QUALIFIERS) is empty. Then, you can show which qualifiers are invalid (helpful if it's a typo and there are many qualifiers).

swh/model/tests/test_identifiers.py
1062–1071

what does "off" mean?

and you can move # noqa at the end of the list so you don't have to repeat it, iirc

Build is green

Patch application report for D4461 (id=15827)

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

Updating 4e3fdc0..1a71edf
Fast-forward
 swh/model/identifiers.py            | 35 ++++++++++++-----
 swh/model/tests/test_identifiers.py | 77 ++++++++++++++++++++++---------------
 2 files changed, 72 insertions(+), 40 deletions(-)
Changes applied before test
commit 1a71edf9830257f300605bebaf097b89b52e4d8a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 12 10:08:45 2020 +0100

    identifiers.parse_swhid: Check the swhid qualifiers and fail if invalid
    
    Related to T2769

commit 22c7c888870986e9303554184f42d4429d4b2ba2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Nov 10 18:21:01 2020 +0100

    model.identifiers: Improve error messages in case of invalid SWHIDs
    
    Related to T2769

commit 47946aac0220fcfd55c7998908c71a6f402a5ddb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Nov 10 16:11:32 2020 +0100

    test: Migrate parse_swhid test cases to pytest
    
    Related to T2769

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

ardumont added inline comments.Thu, Nov 12, 11:20 AM
swh/model/tests/test_identifiers.py
1062–1071

off as in off course so in the end, i mean`wrong` but it's 3 characters more to typ ¯\_(ツ)_/¯

i'll check the noqa hint (i'm not sure it works)

thanks.

ardumont updated this revision to Diff 15829.Thu, Nov 12, 11:29 AM

Adapt according to review

swh/model/tests/test_identifiers.py
1062–1071

nah, it complains [1], i gather what you had in mind was for the docstring.

[1]

$ flake8 swh
swh/model/tests/test_identifiers.py:1063:89: E501 line too long (99 > 88 characters)
swh/model/tests/test_identifiers.py:1065:89: E501 line too long (99 > 88 characters)
swh/model/tests/test_identifiers.py:1067:89: E501 line too long (100 > 88 characters)
swh/model/tests/test_identifiers.py:1069:89: E501 line too long (100 > 88 characters)
swh/model/tests/test_identifiers.py:1071:89: E501 line too long (110 > 88 characters)
swh/model/tests/test_identifiers.py:1074:89: E501 line too long (101 > 88 characters)
swh/model/tests/test_identifiers.py:1075:89: E501 line too long (109 > 88 characters)
ardumont marked 2 inline comments as done.Thu, Nov 12, 11:29 AM
ardumont added inline comments.
swh/model/identifiers.py
849–866

thanks!

swh/model/tests/test_identifiers.py
1062–1071

renamed off to wrong.

Build is green

Patch application report for D4461 (id=15829)

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

Updating 4e3fdc0..7136504
Fast-forward
 swh/model/identifiers.py            | 38 +++++++++++++-----
 swh/model/tests/test_identifiers.py | 77 ++++++++++++++++++++++---------------
 2 files changed, 75 insertions(+), 40 deletions(-)
Changes applied before test
commit 71365044b5f386da3827ccc29184d640256fb070
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 12 10:08:45 2020 +0100

    identifiers.parse_swhid: Check the swhid qualifiers and fail if invalid
    
    Related to T2769

commit 22c7c888870986e9303554184f42d4429d4b2ba2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Nov 10 18:21:01 2020 +0100

    model.identifiers: Improve error messages in case of invalid SWHIDs
    
    Related to T2769

commit 47946aac0220fcfd55c7998908c71a6f402a5ddb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Nov 10 16:11:32 2020 +0100

    test: Migrate parse_swhid test cases to pytest
    
    Related to T2769

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

vlorentz added inline comments.Thu, Nov 12, 11:58 AM
swh/model/identifiers.py
863

s/" The qualifier must be one of/"The qualifiers must be/

(space + plural)

ardumont updated this revision to Diff 15836.Thu, Nov 12, 12:38 PM
ardumont marked an inline comment as done.

Fix space

(the singular was voluntary but ok, it's clearer since i've used the
plural on the first sentence anyway)

Build is green

Patch application report for D4461 (id=15836)

Rebasing onto 22c7c88887...

Current branch diff-target is up to date.
Changes applied before test
commit fb504b400ba90fe8a0815769b6a431489fc8a560
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 12 10:08:45 2020 +0100

    identifiers.parse_swhid: Check the swhid qualifiers and fail if invalid
    
    Related to T2769

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

vlorentz accepted this revision.Thu, Nov 12, 12:56 PM
This revision is now accepted and ready to land.Thu, Nov 12, 12:56 PM