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

Bug 1650787 - Implement JWE metric in the bindings #1073

Merged
merged 11 commits into from
Jul 21, 2020

Conversation

brizental
Copy link
Contributor

No description provided.

Copy link
Contributor

@Dexterp37 Dexterp37 left a comment

Choose a reason for hiding this comment

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

The same consideration apply to all the bindings!

Copy link
Contributor

@Dexterp37 Dexterp37 left a comment

Choose a reason for hiding this comment

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

r+ on all the bindings except Swift :) - please flag a Swift peer for reviewing the Swift parts.

Copy link
Contributor

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

Looks great! Holy smokes it's a lot of work to add one of these now ;)

Just a question about the JSON result, really...

@badboy badboy removed their request for review July 21, 2020 09:33
* Return `JweData` struct from test_get_value (all bindings);
* Add explicit test for `test_get_value` for each binding;
* Fix docstring for python get_test_* functions.
Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

My main concern that I felt required some changes is where you are using force casts. If you could at least reassure me that it could never fail, I'll approve this. Other than that, this looks great from a Swift perspective!

glean-core/ios/Glean/Metrics/JweMetric.swift Outdated Show resolved Hide resolved
glean-core/ios/Glean/Metrics/JweMetric.swift Outdated Show resolved Hide resolved
glean-core/ios/Glean/Metrics/JweMetric.swift Show resolved Hide resolved
glean-core/ios/Glean/Metrics/JweMetric.swift Show resolved Hide resolved
glean-core/ios/GleanTests/Metrics/JweMetricTests.swift Outdated Show resolved Hide resolved
@brizental brizental merged commit 90ce533 into mozilla:main Jul 21, 2020
@brizental brizental deleted the 1650787-jwe-2 branch July 21, 2020 15:22
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.

4 participants