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

update yar to use request.yar instead of request.session #89

Merged
merged 3 commits into from Jan 5, 2016
Merged

Conversation

@mark-bradshaw
Copy link
Contributor

mark-bradshaw commented Jan 5, 2016

This takes care of #88. request.session is changed to request.yar. This is a breaking change.

@mark-bradshaw mark-bradshaw self-assigned this Jan 5, 2016
@mark-bradshaw mark-bradshaw added this to the 6.0.0 milestone Jan 5, 2016
@mark-bradshaw

This comment has been minimized.

Copy link
Contributor Author

mark-bradshaw commented Jan 5, 2016

@devinivy or @nlf, want to do a once over on this PR?

@@ -1197,7 +1197,7 @@ describe('flash()', () => {

server.inject({ method: 'GET', url: '/2', headers: { cookie: cookie[1] } }, (res2) => {

expect(res2.result.session._flash.error).to.not.exist();
expect(res2.result._flash).to.not.exist();

This comment has been minimized.

Copy link
@devinivy

devinivy Jan 5, 2016

Member

I don't think this required any change. res2.result._flash is guaranteed not to exist based upon how the handler replies. _flash lives on request.yar._store. Other tests that reply with reply(request.yar._store) do correctly check res.result._flash, which may be the reason this change was made.

@@ -1133,7 +1133,7 @@ describe('flash()', () => {

server.inject({ method: 'GET', url: '/2', headers: { cookie: cookie[1] } }, (res2) => {

expect(res2.result.session._flash.error).to.not.exist();

This comment has been minimized.

Copy link
@devinivy

devinivy Jan 5, 2016

Member

Ah, here is the same change again. Maybe I've misunderstood something! That said, it does seem to me that the change is not necessary and creates a "moot" assertion.

@devinivy

This comment has been minimized.

Copy link
Member

devinivy commented Jan 5, 2016

Did my once-over! Looks good– I just got hung-up on a couple changes in the tests.

@mark-bradshaw

This comment has been minimized.

Copy link
Contributor Author

mark-bradshaw commented Jan 5, 2016

Thanks Devin. I'll take a look at that tomorrow.
On Mon, Jan 4, 2016 at 11:42 PM devin ivy notifications@github.com wrote:

Did my once-over! Looks good– I just got hung-up on a couple changes in
the tests.


Reply to this email directly or view it on GitHub
#89 (comment).

mark-bradshaw added a commit that referenced this pull request Jan 5, 2016
update yar to use request.yar instead of request.session
@mark-bradshaw mark-bradshaw merged commit 915c8bb into master Jan 5, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@mark-bradshaw mark-bradshaw deleted the session branch Jan 5, 2016
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Jan 10, 2016

I don't see you using server.decorate anywhere? Wasn't that the intent of this PR and what Eran suggested changing?

Never mind, just saw #91. As you were :-)

@mark-bradshaw

This comment has been minimized.

Copy link
Contributor Author

mark-bradshaw commented Jan 11, 2016

It's in follow up code.
On Sun, Jan 10, 2016 at 5:12 AM Adam Bretz notifications@github.com wrote:

I don't see you using server.decorate anywhere? Wasn't that the intent of
this PR and what Eran suggested changing?


Reply to this email directly or view it on GitHub
#89 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.