Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2548 +/- ##
==========================================
- Coverage 69.25% 64.01% -5.25%
==========================================
Files 126 123 -3
Lines 4759 4707 -52
==========================================
- Hits 3296 3013 -283
- Misses 1463 1694 +231
|
56ebdce to
5a73d4a
Compare
1bee091 to
6d8f774
Compare
| if (argument.begin || argument.end).is_a?(ActiveSupport::TimeWithZone) | ||
| argument.to_s | ||
| else | ||
| argument.map { |v| sentry_serialize_arguments(v) } |
There was a problem hiding this comment.
Do we need to serialize individual components in the range? If the range is (1..10000) then it'd probably be pretty slow AND it'd allocate a new array with 10k elements in it.
Maybe in this case we just leave the argument until we find another element type that requires special treatments?
There was a problem hiding this comment.
@st0012 we don't but it's the original behavior - is it OK to change it in a patch/bugfix release?
There was a problem hiding this comment.
Ah is it because it was entering the Enumerable branch? In that case, let's leave it as it is.
d3a6db5 to
e7db3e1
Compare
If a range consists for ActiveSupport::TimeWithZone, it will be serialized as a string. Previously it would raise an error ``` TypeError: can't iterate from ActiveSupport::TimeWithZone ``` Closes #2545
e7db3e1 to
bc76440
Compare
|
This PR introduced a new challenge where we have specs that now have min Ruby version requirements due to syntax that's used (end-less and begin-less ranges in this case). To solve this, I introduced a new rake task called @sl0thentr0py @st0012 are you OK with such a solution? One remaining bit is to figure out what to do with Rubocop that has Lint/Syntax and target ruby version set to 2.6, so now it fails. |
|
let's just not use fancy syntax please then |
oh, it's not that - we just need verify that such ranges work with activejob context param serialization 😅 |
|
I assume a simple |
hah yeah that was my instinct but then I realized that ruby parses entire file first, it won't skip anything due to conditionals so we were ending up with syntax errors. Alright, gonna merge this in, FWIW I think it's gonna be useful for other stuff too. |
ed9cba9 to
a56bbf8
Compare
Our target ruby version is 2.6 so...
a56bbf8 to
1b9344a
Compare
If a range consists for ActiveSupport::TimeWithZone,
it will be serialized as a string.
Previously it would raise an error
Closes #2545