-
-
Notifications
You must be signed in to change notification settings - Fork 124
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
Issue 90: Add additional instrumentation for flash messages. #173
Merged
Merged
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
7e897c8
Add additional instrumentation for flash messages.
jsthomas 4dc6623
Adjust whitespace.
jsthomas 2010aad
Adjust whitespace.
jsthomas bd2c8b3
Revise indentation.
jsthomas 957a09f
Move log point before inner_handler call.
jsthomas fa30003
Remove use of initialize_log in example.
jsthomas 7321682
Save a round-trip on trivial nits
aantron File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this display the contents of
outbox
, rather than reading the cookie, which AFAIK is the flash messages returned in the request? If you'd like to display incoming flash messages as I think this is doing, I think it should be done before the call toinner_handler
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the issue, @thangngoc89 wrote that it was "frustrating to debug why flash messages aren't rendered". I thought it made sense to log the flash messages from the current request's cookie because those are the ones available for rendering a response. This also appears to match the behavior of the example code in the issue description.
I've fixed the ordering with
inner_handler
; that was an oversight on my part.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question is, in the original ordering, why read the cookie on the request (
flash request
), rather than readingoutbox
? Aren't the messages in the outbox the ones that will be put in the response?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, I understood you. I got double-negative-confused by myself :) Okay, I now think this is doing what #90 was asking, and the ordering makes sense now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move the debug print all the way to the top of the middleware? It doesn't seem to depend on any of the values bound before it now, and having them bound there makes it seem like it might depend. It's a clean print of the flash messages being returned to the server. It will especially make it clear that the logging does not depend on the rebinding of
request
to one that has theoutbox
attached.