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

cookies: is &mut self required? #317

Merged
merged 1 commit into from
Oct 24, 2019
Merged

Conversation

secretfader
Copy link

@secretfader secretfader commented Aug 23, 2019

&mut self doesn't seem to be required, as mentioned in #316.

Description

Change cookie trait function signature on get_cookie from &mut self to &self.

Motivation and Context

Tide seems to compile fine without requiring a mutable reference. Unsure why the previous behavior may have been required, but I believe there's little need to require mutability when reading cookie values.

How Has This Been Tested?

Existing tests passed.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Sorry, something went wrong.

@secretfader secretfader force-pushed the fix-gh-316 branch 2 times, most recently from f911c8d to 0a32c58 Compare August 23, 2019 17:45
@secretfader secretfader requested review from taiki-e and Nemo157 August 23, 2019 18:07
@kamyuentse
Copy link

I think we should keep&mut self for set|remove_cookie.

@secretfader secretfader force-pushed the fix-gh-316 branch 2 times, most recently from 8733860 to 40b612a Compare August 24, 2019 17:51
@secretfader
Copy link
Author

Ping @rustasync/maintainers

@secretfader
Copy link
Author

This would be a breaking change, but I agree with the assertion in #316: that &mut self doesn't make sense here.

@secretfader
Copy link
Author

I'm slightly concerned with merging this breaking API change, even though I believe it to be technically more correct. As a consumer of Tide and owner of several applications that will be broken when this change is released, and believe the update is worth applying.

I'll go ahead and merge since I'm no longer the only one frustrated (see #316, where the concern is similar to mine: building authentication middleware). Feel free to comment (or revert) if you feel strongly about requiring mut. I think dropping the requirement will make extracting data much cleaner.

@secretfader secretfader merged commit 6bd69a2 into http-rs:master Oct 24, 2019
@secretfader secretfader deleted the fix-gh-316 branch October 24, 2019 20:11
yoshuawuyts added a commit that referenced this pull request Nov 3, 2019

Unverified

This user has not yet uploaded their public signing key.
Signed-off-by: Yoshua Wuyts <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants