Skip to content

Is method-specific-id supposed to be equivalent to param-char? #45

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
brentzundel opened this issue Sep 23, 2019 · 8 comments
Closed

Is method-specific-id supposed to be equivalent to param-char? #45

brentzundel opened this issue Sep 23, 2019 · 8 comments
Assignees
Labels
editorial The change is expected to be an editorial change pending close Issue will be closed shortly if no objections

Comments

@brentzundel
Copy link
Member

@aljones15 moved from CCG (w3c-ccg/did-spec#223)

@msporny msporny added the editorial The change is expected to be an editorial change label Oct 1, 2019
@msporny
Copy link
Member

msporny commented Oct 1, 2019

@talltree and @peacekeeper can you please take a look at the ABNF simplification that @aljones15 is suggesting in w3c-ccg/did-spec#223 ? If it looks good to you, this is editorial and one of the Editors can make the change.

@aljones15
Copy link
Contributor

aljones15 commented Jan 27, 2020

just to make it easier to see the issue I am copying the content here:

method-specific-id = *idchar *( ":" *idchar )
idchar             = ALPHA / DIGIT / "." / "-" / "_"
param-char         = ALPHA / DIGIT / "." / "-" / "_" / ":" /
                     pct-encoded

if you remove the pct-encoded from param-char then method-specific-id appears to be equivalent to method-specific-id.

method-specific-id = *idchar *( ":" *idchar )

this line makes the 2 equivalent because method-specific-id does include ":" inside of it with 0 or more idchars.

hence a valid method-specific-id could be:

method-specific-id = *did-char
did-char = ALPHA / DIGIT / "." / "-" / "_" / ":" /
param-char = did-char pct-encoded

param-char is much easier to read as it uses Alternatives to describe it's strings.
method-specific-id's sequence group seems like an artifact from when param-char and method-specific-id differed or perhaps is reserved for future use if it becomes necessary to make it different from param-char.
Basically we could define one rule for both method-specific-id and param-char and then specify that param-char should be pct-encoded.

additionally idchar is inconsistently named because the other ABNF rules are snake-case.

I guess it feels like idchar and param-char are/were supposed to be different strings, but it was generalized to the point that the two are effectively identical.

@talltree
Copy link
Contributor

@aljones15 Sorry it took me so long to get to this. (Any ABNF discussion always requires a little flow time to analyze.)

Having taken that time, I agree with you, I don't see any issues with this enhancement and it does indeed simplify the ABNF.

Before closing this issue, I'd like @peacekeeper to also review it and see if he agrees. If so, Markus, please tag it as Ready for PR.

@peacekeeper
Copy link
Contributor

@aljones15 when you say

param-char = did-char pct-encoded

I believe you really mean

param-char = did-char / pct-encoded

right?

@peacekeeper
Copy link
Contributor

Personally I don't think eliminating all repetition in ABNF is always a good thing, sometimes it's "easier to read" if the rules are more verbose. I think my preferred approach would be the following, which simplifies method-specific-id and idchar, but leaves param-char unchanged:

method-specific-id = *id-char
id-char = ALPHA / DIGIT / "." / "-" / "_" / ":"

...

param-char = ALPHA / DIGIT / "." / "-" / "_" / ":" / pct-encoded

But I don't feel strongly about it, I'm also fine with doing what @aljones15 proposed!

@peacekeeper peacekeeper added the ready for pr Issue is ready for a PR label Feb 26, 2020
@rhiaro rhiaro added pr exists There is an open PR to address this issue and removed ready for pr Issue is ready for a PR labels Mar 16, 2020
@peacekeeper
Copy link
Contributor

additionally idchar is inconsistently named because the other ABNF rules are snake-case.

After reviewing this again, I now believe that idchar is preferable over id-char, since this is aligned with the pchar rule in RFC3986.

And as I already mentioned above, while @aljones15 's proposal could remove some repetition in the ABNF, personally I don't feel like that actually makes it easier to read. I prefer to keep the rules idchar and param-char separate, even if they are very similar.

Therefore I propose to close this issue.

@OR13
Copy link
Contributor

OR13 commented Mar 17, 2020

I'm in favor of closing this issue as well.

@peacekeeper peacekeeper removed the pr exists There is an open PR to address this issue label Mar 17, 2020
@peacekeeper peacekeeper added the pending close Issue will be closed shortly if no objections label May 12, 2020
@peacekeeper
Copy link
Contributor

@aljones15 since we haven't heard back from you after some discussion, I am going to close this, but feel free to simply open a new issue if you still think the current ABNF can/should be improved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial The change is expected to be an editorial change pending close Issue will be closed shortly if no objections
Projects
None yet
Development

No branches or pull requests

7 participants