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.

Sign up for GitHub

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

Jump to bottom

Issue #1340 - Update cookie size requirements #1563

Merged
merged 3 commits into from
Jun 25, 2021
Merged

Issue #1340 - Update cookie size requirements #1563

merged 3 commits into from
Jun 25, 2021

Conversation

miketaylr
Copy link
Collaborator

@chlily1 do you think we should move these requirements into the Set-Cookie parsing algorithm and make them MUST NOT instead of SHOULD NOT? Or fine to leave as-is?

@miketaylr
Copy link
Collaborator Author

(It feels easier to write tests against MUST vs SHOULD)

@miketaylr
Copy link
Collaborator Author

Actually, yeah. Let's do that. I'll update this PR.

@miketaylr miketaylr marked this pull request as ready for review June 23, 2021 18:37
@miketaylr
Copy link
Collaborator Author

PTAL @chlily1

@miketaylr miketaylr requested a review from chlily1 June 24, 2021 15:26
chlily1
chlily1 reviewed Jun 24, 2021
View reviewed changes
Copy link
Contributor

@chlily1 chlily1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Algorithm changes look good. Had a suggestion for the "Limits" section for clarity.

draft-ietf-httpbis-rfc6265bis.md Show resolved Hide resolved
@miketaylr miketaylr requested a review from chlily1 June 24, 2021 19:54
@miketaylr miketaylr requested a review from chlily1 June 24, 2021 22:58
@chlily1
Copy link
Contributor

chlily1 commented Jun 25, 2021

Thanks! LGTM.

@chlily1 chlily1 merged commit 507f2b7 into httpwg:main Jun 25, 2021
@chlily1 chlily1 mentioned this pull request Jun 25, 2021
Closed
@miketaylr miketaylr deleted the issues/1340/2 branch June 25, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chlily1 chlily1 Awaiting requested review from chlily1

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@miketaylr @chlily1