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

Rename clearingLang to withoutLang #8833

Merged
merged 3 commits into from Dec 5, 2018

Conversation

Projects
None yet
4 participants
@gmethvin
Copy link
Member

gmethvin commented Nov 27, 2018

clearingLang "clears" the language cookie, but the actual effect it has is that it resets to the default lang based on the Accept-Language header, so I think withDefaultLang is a better name. This requires deprecating the existing Scala API.

@gmethvin

This comment has been minimized.

Copy link
Member Author

gmethvin commented Nov 27, 2018

Thinking about it more, I'm wondering if withDefaultLang could be confusing. "default" could mean different things depending on the context. withoutLang might be clearer, but not much more so than clearingLang. Maybe it's not worth the change.

@marcospereira

This comment has been minimized.

Copy link
Member

marcospereira commented Nov 28, 2018

withoutLang is at least more consistent with withLang. :-)

@marcospereira
Copy link
Member

marcospereira left a comment

Thank you, @gmethvin.

As I said at the other comment, I think that using withoutLang makes more sense here. Of course this is API polishing so just a marginal gain here, but still, worth to do it.

LGTM after this change.

@gmethvin gmethvin changed the title Rename clearingLang to withDefaultLang Rename clearingLang to withoutLang Nov 30, 2018

@gmethvin gmethvin force-pushed the gmethvin:cookie-lang branch 2 times, most recently from c18c42f to a77558b Nov 30, 2018

@gmethvin gmethvin force-pushed the gmethvin:cookie-lang branch 2 times, most recently from 73a1234 to bede79e Nov 30, 2018

@gmethvin

This comment has been minimized.

Copy link
Member Author

gmethvin commented Nov 30, 2018

sure :) done.

@dwijnand dwijnand added this to the Play 2.7.0 milestone Dec 4, 2018

@gmethvin

This comment has been minimized.

Copy link
Member Author

gmethvin commented Dec 5, 2018

@marcospereira are we ready to merge this?

@marcospereira
Copy link
Member

marcospereira left a comment

LGTM.

Let's merge after a green build.

@marcospereira marcospereira merged commit 66f112a into playframework:master Dec 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

marcospereira added a commit that referenced this pull request Dec 5, 2018

@marcospereira

This comment has been minimized.

Copy link
Member

marcospereira commented Dec 5, 2018

Backport to 2.7.x: ba1a04c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment