Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

[glean] 1535083: Use enums as extra keys on events #2403

Merged
merged 1 commit into from
Mar 20, 2019

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Mar 19, 2019

This uses enums for the extra keys on events so they are checked at build time rather than run time.

This won't pass CI until mozilla/glean_parser#44 is merged and this PR is updated to use that.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

@mdboom mdboom requested review from Dexterp37 and travis79 March 19, 2019 13:10
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.

LGTM! Let's merge this after tests have passed (pending the glean_parsers thing)

// a template parameter).
if (it.size > 0) {
val cls = it.keys.first().javaClass
val method = cls.getDeclaredMethod("getValue")
Copy link
Contributor

Choose a reason for hiding this comment

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

Eugh, I hoped we could make it without it. But we can't :) No problem!

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.

LGTM, although I wish there was a way to avoid reflection.

@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #2403 into master will decrease coverage by 0.15%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2403      +/-   ##
============================================
- Coverage     83.74%   83.58%   -0.16%     
+ Complexity     2729     2672      -57     
============================================
  Files           334      325       -9     
  Lines         11355    11216     -139     
  Branches       1658     1637      -21     
============================================
- Hits           9509     9375     -134     
- Misses         1163     1166       +3     
+ Partials        683      675       -8
Impacted Files Coverage Δ Complexity Δ
...ents/service/glean/storages/EventsStorageEngine.kt 83.01% <ø> (-14%) 15 <0> (-3)
...ozilla/components/service/glean/EventMetricType.kt 85.29% <75%> (-7.3%) 16 <3> (ø)
...mponents/service/glean/debug/GleanDebugActivity.kt 100% <0%> (ø) 4% <0%> (+1%) ⬆️
...la/components/feature/session/FullScreenFeature.kt
...ozilla/components/feature/session/WindowFeature.kt
...la/components/feature/session/ThumbnailsFeature.kt
...illa/components/feature/session/SessionUseCases.kt
...ponents/feature/session/PictureInPictureFeature.kt
...illa/components/feature/session/HistoryDelegate.kt
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 653d2aa...b8f2194. Read the comment docs.

@mdboom mdboom marked this pull request as ready for review March 20, 2019 14:06
@mdboom mdboom requested a review from a team as a code owner March 20, 2019 14:06
@mdboom mdboom merged commit 91c0e4d into mozilla-mobile:master Mar 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants