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

less verbose/redundant mandatory extension boilerplate #236

Closed
wants to merge 1 commit into from

Conversation

davegarrett
Copy link
Contributor

After reading through the new parts of the published v08 draft, I think my PR to clean up the mandatory extension handling was a little too verbose and had too many repeated sentences. This refactors it a little bit to just explicitly define two terms, MANDATORY TO SEND and CLIENT ONLY, and just use them where applicable. (all caps to be clear and quick to see on skim, similar to RFC 2119 terminology) It's basically the same language, just generalized and not repeated 3 or 4 times verbatim.

The one repeated sentence could probably be shorter, but I think it's more clear when stated explicitly for each section.

Overall, there's less text now and each extension's section is significantly less verbose.

@davegarrett davegarrett force-pushed the extensionboilerplate branch 2 times, most recently from 839d1fc to 430b41b Compare September 17, 2015 17:01
@davegarrett
Copy link
Contributor Author

rebased again

@ekr: This is just an editorial cleanup of my recent additions. Please take a look at this at some point (and similarly PR #234) to see if it looks good or needs changes.

@davegarrett
Copy link
Contributor Author

Yes, that idea does sound better than doing it in paragraphs. I'll give redoing it like that a shot. I'll update this PR when I have something.

@davegarrett davegarrett force-pushed the extensionboilerplate branch from 9c73823 to 42efbce Compare October 3, 2015 23:06
@davegarrett
Copy link
Contributor Author

Ok, revised rather significantly. I tried doing the list style heading, but it just didn't seem to work all that well. The biggest reduction in repetition in this is just moving the client-only bit down into the MTI extensions section and just listing the 3 there with the applicable requirements. This version here is shorter than the current draft, but not as short as the previous version of this PR. More clear to read, overall, though.

Edited to note:
PR #214 is now rebased on top of this updated PR

@davegarrett
Copy link
Contributor Author

The text here assumes omitting the ClientKeyShare is allowed, by the way. Landing that PR first is probably best, after which I'll fix the conflicts here.

@davegarrett
Copy link
Contributor Author

merged here:
bdf8993
and tweaked more here:
07b18eb

Closing.

@davegarrett davegarrett deleted the extensionboilerplate branch October 12, 2015 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants