Skip to content
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

[SH] use RWS in inner-list #983

Closed
wants to merge 1 commit into from

Conversation

phluid61
Copy link
Collaborator

@phluid61 phluid61 commented Nov 8, 2019

  • update ABNF for inner-list to use RWS
  • update parsing algorithm to accept HTAB
  • update imports to include RWS and HTAB

For #961

* update ABNF for inner-list to use RWS
* update parsing algorithm to accept HTAB
* update imports to include RWS and HTAB

For httpwg#961
@reschke
Copy link
Contributor

reschke commented Nov 8, 2019

RWS implies "required". Is that really the intent?

@phluid61
Copy link
Collaborator Author

phluid61 commented Nov 8, 2019

Yes. We require spaces between the items in an inner-list, otherwise they can abut.

@phluid61 phluid61 changed the title use RWS in inner-list [SH] use RWS in inner-list Nov 8, 2019
@mnot mnot mentioned this pull request Dec 4, 2019
@ioggstream
Copy link
Contributor

imho RWS and TABs make things error-prone and difficult to debug.

@phluid61
Copy link
Collaborator Author

phluid61 commented Dec 4, 2019

Just so we're aware, this PR was to address an issue identified in #961 where the ABNF says OWS (including htab) but the reference algorithm requires SP before OWS after an sh-item.

So changing the ABNF to match the algorithm gives:

inner-list = "(" OWS [ sh-item *( SP OWS sh-item ) [ SP OWS ] ] ")" *parameter

Alternatively, by extending the algorithm to also accept a htab immediately after an item (this PR), the ABNF becomes:

inner-list = "(" OWS [ sh-item *( RWS sh-item ) OWS ")" *parameter

An alternative is to remove the htab altogether, which removes OWS, so we're left with:

inner-list = "(" *SP [ sh-item *( 1*SP sh-item ) *SP ")" *parameter

Which is fine by me, but removing all OWS is a bigger change.

@kazuho
Copy link
Contributor

kazuho commented Dec 5, 2019

I think we should retain the use of OWS, rather than removing them or changing (some of) them to RWS.

Not all SH header fields would be generated by a serializer that understands SH. Rather, I'd assume that most of SH header fields would be written by hand, or generated by concatenating strings (in various programming languages).

I do not think we can easily enforce everyone to insert whitespaces is a particular way (or drop them).

@mnot
Copy link
Member

mnot commented Dec 5, 2019

@kazuho I'm not as worried about that; tabs in existing headers are extremely uncommon, IME (looking at things like the HTTP Archive dump). If SH implementations are consistent (and they should be if we test well), this will get caught early.

@kazuho
Copy link
Contributor

kazuho commented Dec 5, 2019

@mnot I'm not worried about tabs. I think it would be fine if the change is going to be either or both of the following and nothing more.

  • OWS -> *SP
  • RWS -> 1*SP

@mnot
Copy link
Member

mnot commented Dec 5, 2019

@kazuho yep +1

@phluid61
Copy link
Collaborator Author

phluid61 commented Dec 5, 2019

I made a new branch (with PR) to see what sort of change is involved replacing OWS with *SP. It's not as invasive as I'd thought it might be.

@mnot
Copy link
Member

mnot commented Dec 11, 2019

OBE

@mnot mnot closed this Dec 11, 2019
@phluid61 phluid61 deleted the 961-RWS-inner-list branch December 11, 2019 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants