Fix regex substitution for warden and empty? errors for Rack
#88
Conversation
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
paulosman
reviewed
May 12, 2020
| @@ -1,7 +1,7 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| module Honeycomb | |||
| # Methods for extracing common warden/devise fields from a rack env hash | |||
| # Methods for extracting common warden/devise fields from a rack env hash | |||
martin308
reviewed
May 12, 2020
| @@ -51,7 +51,7 @@ def last_name | |||
| end | |||
|
|
|||
| def created_at | |||
| Time.now | |||
| Time.new 2002, 3, 4, 5, 6, 7 | |||
There was a problem hiding this comment.
No specific issue, just a general aversion to having variable dates in tests; I've fixed too many tests that flaked out due to time quirks. I can back it out if you want.
paulosman
approved these changes
May 12, 2020
There was a problem hiding this comment.
LGTM but would like @martin308 to review before deciding whether to merge.
martin308
approved these changes
May 12, 2020
There was a problem hiding this comment.
@irvingreid looks good. I pulled the code down and had a play and everything makes sense now!
|
Do you still want two separate PRs or is this one OK? |
|
@irvingreid I think we could pull one or two small things out but they are much smaller than I initially thought so let's just use this one. Sorry for the confusion and thanks for bearing with us |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Three things I found in testing the Rails beeline with our application:
empty?(); Ruby only implements that method on collection types.falsevalue. If this is intentional, fine, though I prefer to include it; there's a meaningful difference between "I don't have a value for that field" and "I have the valuefalsefor that field".