Page MenuHomeSoftware Heritage

model.identifiers: Improve error messages in case of invalid SWHIDs
ClosedPublic

Authored by ardumont on Tue, Nov 10, 6:20 PM.

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.Tue, Nov 10, 6:20 PM

Build has FAILED

Patch application report for D4458 (id=15804)

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

Updating 4e3fdc0..f398731
Fast-forward
 swh/model/identifiers.py            |  49 +++++++++++++----
 swh/model/tests/test_identifiers.py | 103 +++++++++++++++++++++++++-----------
 2 files changed, 111 insertions(+), 41 deletions(-)
Changes applied before test
commit f3987316c42817f0586cc637437c5b4eb879e3b7
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
    
    As a proposal, the grammar was inlined within the error message.
    
    Related to T2769

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

    test: Check the detailed error message when swhid is invalid
    
    Related to T2769

Link to build: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/162/
See console output for more information: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/162/console

ardumont updated this revision to Diff 15806.Tue, Nov 10, 6:38 PM

Rebase

(tests should pass now)

ardumont edited the test plan for this revision. (Show Details)Tue, Nov 10, 6:38 PM

Build is green

Patch application report for D4458 (id=15806)

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

Updating 4e3fdc0..bc8be0f
Fast-forward
 swh/model/identifiers.py            | 49 +++++++++++++++++++++------
 swh/model/tests/test_identifiers.py | 67 ++++++++++++++++++++-----------------
 2 files changed, 75 insertions(+), 41 deletions(-)
Changes applied before test
commit bc8be0f86ebc12336ef31240b56befa714eefbea
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
    
    As a proposal, the grammar was inlined within the error message.
    
    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/164/ for more details.

ardumont edited the summary of this revision. (Show Details)Wed, Nov 11, 10:27 AM
ardumont updated this revision to Diff 15815.Thu, Nov 12, 10:15 AM
  • Add missing partial grammar on qualifier
  • rework commit message

Build is green

Patch application report for D4458 (id=15815)

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

Updating 4e3fdc0..8707175
Fast-forward
 swh/model/identifiers.py            | 54 ++++++++++++++++++++++++------
 swh/model/tests/test_identifiers.py | 67 ++++++++++++++++++++-----------------
 2 files changed, 80 insertions(+), 41 deletions(-)
Changes applied before test
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/165/ for more details.

ardumont updated this revision to Diff 15820.Thu, Nov 12, 10:26 AM

Add again some grammar part, now it should be complete

Build is green

Patch application report for D4458 (id=15820)

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

Updating 4e3fdc0..cb11afe
Fast-forward
 swh/model/identifiers.py            | 58 ++++++++++++++++++++++++++------
 swh/model/tests/test_identifiers.py | 67 ++++++++++++++++++++-----------------
 2 files changed, 84 insertions(+), 41 deletions(-)
Changes applied before test
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/168/ for more details.

zack requested changes to this revision.Thu, Nov 12, 10:44 AM
zack added a subscriber: zack.
zack added inline comments.
swh/model/identifiers.py
731

better: "Invalid SWHID: namespace is '{value}' but must be '{SWHID_NAMESPACE}'"

(note how this also shows where the error is)

739

"Invalid SWHID: version is {value} but must be {SWHID_VERSION}"

749

"Invalid SWHID: object type is {value} but must be one {supported_types}"

(you can also move the join inside the {} and avoid an extra variable, but that's a minor detail)

829

Duplicating the EBNF grammar here is a DRY violation, given it's already under docs/ and if you put it here too there will be two places to be changed in case of evolution.

But aside from that, I think a meaningful error for users here could be much simpler, as they just need (here) to know there should be 4 values. So how about: "Invalid SWHID, format must be 'swh:1:OBJECT_TYPE:OBJECT_ID'". I think that would convey anything the user needs to know.

850–851

this is a more tricky one, but my point about duplicating the grammar still applies. So, first attempt:

"Invalid SWHID: contextual data must be a ;-separated list of key=value pairs."

This revision now requires changes to proceed.Thu, Nov 12, 10:44 AM

Thanks.

swh/model/identifiers.py
749

the intermediary variable is really for readable purpose, to avoid plenty of nested {}().

829

yes, indeed.
I did not see a simpler way though because i was set on exhaustivness...

If we relax a bit the exhaustivness, your proposal is quite fine, thanks.

ardumont updated this revision to Diff 15823.Thu, Nov 12, 11:01 AM

Adapt according to review.

Simplify error messages to a meaningful summary of what's missing (and don't
repeat the grammar as it's in the docs already, be DRY)

Build is green

Patch application report for D4458 (id=15823)

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

Updating 4e3fdc0..8130a27
Fast-forward
 swh/model/identifiers.py            | 22 ++++++++----
 swh/model/tests/test_identifiers.py | 67 ++++++++++++++++++++-----------------
 2 files changed, 51 insertions(+), 38 deletions(-)
Changes applied before test
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/171/ for more details.

ardumont retitled this revision from model.identifiers: Improve error message by exposing the grammar to model.identifiers: Improve error messages in case of invalid SWHIDs.Thu, Nov 12, 11:03 AM
zack accepted this revision.Thu, Nov 12, 11:09 AM

LGTM !, thanks

(only a couple of minor things noted above, but you can fix it before pushing)

swh/model/identifiers.py
747

oops, this should be "one of" (you inherited my own typo, sorry :))

842–843

minor: a more canonical error message of this kind would be "Invalid SWHID: missing OBJECT_ID"

This revision is now accepted and ready to land.Thu, Nov 12, 11:09 AM
ardumont marked an inline comment as done.Thu, Nov 12, 11:11 AM
ardumont added inline comments.
swh/model/identifiers.py
747

yes, indeed (and the sun on my screen did not help ;)

ardumont edited the summary of this revision. (Show Details)Thu, Nov 12, 11:12 AM
ardumont updated this revision to Diff 15826.Thu, Nov 12, 11:14 AM

Fix typo and improve error message

Build is green

Patch application report for D4458 (id=15826)

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

Updating 4e3fdc0..22c7c88
Fast-forward
 swh/model/identifiers.py            | 22 ++++++++----
 swh/model/tests/test_identifiers.py | 67 ++++++++++++++++++++-----------------
 2 files changed, 51 insertions(+), 38 deletions(-)
Changes applied before test
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/174/ for more details.