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

ardumont created this revision.Nov 10 2020, 6:20 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptNov 10 2020, 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

Harbormaster failed remote builds in B17014: Diff 15804!Nov 10 2020, 6:22 PM

Rebase

(tests should pass now)

ardumont edited the test plan for this revision. (Show Details)Nov 10 2020, 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.

Harbormaster completed remote builds in B17016: Diff 15806.Nov 10 2020, 6:39 PM

ardumont edited the summary of this revision. (Show Details)Nov 11 2020, 10:27 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.

Harbormaster completed remote builds in B17025: Diff 15815.Nov 12 2020, 10:16 AM

ardumont added a child revision: D4461: identifiers.parse_swhid: Check the swhid qualifiers and fail if invalid.Nov 12 2020, 10:16 AM

ardumont edited the summary of this revision. (Show Details)

ardumont added a parent revision: D4457: test: Migrate parse_swhid test cases to pytest.

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.

Harbormaster completed remote builds in B17030: Diff 15820.Nov 12 2020, 10:27 AM

zack requested changes to this revision.Nov 12 2020, 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}"
748	"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)
828	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.Nov 12 2020, 10:44 AM

Thanks.

swh/model/identifiers.py
748	the intermediary variable is really for readable purpose, to avoid plenty of nested {}().
828	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.

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.

Harbormaster completed remote builds in B17033: Diff 15823.Nov 12 2020, 11:03 AM

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.Nov 12 2020, 11:03 AM

ardumont mentioned this in D4462: identifiers.parse_swhid: Make SWHIDs with whitespaces invalid.Nov 12 2020, 11:06 AM

LGTM !, thanks

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

swh/model/identifiers.py
746	oops, this should be "one of" (you inherited my own typo, sorry :))
841–842	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.Nov 12 2020, 11:09 AM

ardumont marked an inline comment as done.Nov 12 2020, 11:11 AM

ardumont added inline comments.

swh/model/identifiers.py
746	yes, indeed (and the sun on my screen did not help ;)

ardumont edited the summary of this revision. (Show Details)Nov 12 2020, 11:12 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.

Harbormaster completed remote builds in B17036: Diff 15826.Nov 12 2020, 11:15 AM

Closed by commit rDMOD22c7c8888709: model.identifiers: Improve error messages in case of invalid SWHIDs (authored by ardumont). · Explain WhyNov 12 2020, 11:39 AM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDMOD22c7c8888709: model.identifiers: Improve error messages in case of invalid SWHIDs.

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

Details

Diff Detail

Event Timeline

Patch application report for D4458 (id=15804)

Changes applied before test

Patch application report for D4458 (id=15806)

Changes applied before test

Patch application report for D4458 (id=15815)

Changes applied before test

Patch application report for D4458 (id=15820)

Changes applied before test

Patch application report for D4458 (id=15823)

Changes applied before test

Patch application report for D4458 (id=15826)

Changes applied before test

Revision Contents
Changeset List

Diff 15833

swh/model/identifiers.py

model.identifiers: Improve error messages in case of invalid SWHIDsClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D4458 (id=15804)

Changes applied before test

Patch application report for D4458 (id=15806)

Changes applied before test

Patch application report for D4458 (id=15815)

Changes applied before test

Patch application report for D4458 (id=15820)

Changes applied before test

Patch application report for D4458 (id=15823)

Changes applied before test

Patch application report for D4458 (id=15826)

Changes applied before test

Revision ContentsChangeset List

Diff 15833

swh/model/identifiers.py

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

Revision Contents
Changeset List