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

feat(reports) custom immutable items #3180

Merged
merged 1 commit into from
Jan 26, 2018

Conversation

gszr
Copy link
Member

@gszr gszr commented Jan 25, 2018

Summary

Add function to insert custom items in the immutable
portion of the report buffer.

@gszr gszr requested a review from thibaultcha January 25, 2018 22:48
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worried about the missing v ~= nil condition, and a little bit about missing tests (there are unit tests for the phone home buffer in phone_home_spec.lua :)

end

return v
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: before merging this, will you update this so we respect the 2 line jumps rule around this function? Thanks!

@@ -181,6 +185,15 @@ local function add_ping_value(k, v)
end


local function add_immutable_value(k, v)
v = get_report_value(v)
if v then
Copy link
Member

@thibaultcha thibaultcha Jan 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be if v ~= nil, otherwise we don't respect the above behavior of reports, and we could end up missing some values when they are false.

v = get_report_value(v)
if v then
_buffer[#_buffer + 1] = k .. "=" .. tostring(v)
_buffer_immutable_idx = _buffer_immutable_idx + 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: we could do this increment prior to the insertion and save the #_buffer + 1, but not a blocker.

v = json
end

return v
Copy link
Member

@thibaultcha thibaultcha Jan 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: we could do return v ~= nil and tostring(v) or nil here, and provide a safer API (both appends to the buffer would not have to do the tostring() themselves anymore, nor would any future code change), but not a blocker.

Add function to insert custom items in the immutable
portion of the report buffer.
@gszr gszr force-pushed the feat/reports-custom-immutable-items branch from 5fdea6e to 288a099 Compare January 26, 2018 16:30
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, looking ok now

@gszr gszr merged commit 5b13071 into master Jan 26, 2018
@gszr gszr deleted the feat/reports-custom-immutable-items branch January 26, 2018 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants