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 1727760 - Implement the Url type #1778

Merged
merged 11 commits into from
Sep 7, 2021
Merged

Conversation

Dexterp37
Copy link
Contributor

No description provided.

glean-core/src/metrics/url.rs Outdated Show resolved Hide resolved
Self { meta }
}

fn is_valid_url_scheme(&self, value: String) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe include a link to the url scheme spec, which explains that -, + and . are allowed.

@Dexterp37 Dexterp37 self-assigned this Sep 3, 2021
@Dexterp37 Dexterp37 marked this pull request as ready for review September 3, 2021 14:36
@auto-assign auto-assign bot requested a review from badboy September 3, 2021 14:36
@Dexterp37 Dexterp37 requested review from travis79 and removed request for badboy September 3, 2021 14:37
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.

Looking good! Is there a bug filed for the Swifty bits?

@Dexterp37
Copy link
Contributor Author

Looking good! Is there a bug filed for the Swifty bits?

Filed right now :) https://bugzilla.mozilla.org/show_bug.cgi?id=1729030

I also rebased on the latest main to see if it fixes the tests :)

@Dexterp37 Dexterp37 requested a review from travis79 September 3, 2021 16:05
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.

Just a few nits

Comment on lines 5 to 9
/* This file is based on the tests in the Glean android-components implementation.
*
* Care should be taken to not reorder elements in this file so it will be easier
* to track changes in Glean android-components.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Is this still true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, no.

glean-core/python/glean/metrics/url.py Outdated Show resolved Hide resolved
@@ -121,6 +123,8 @@ pub enum Metric {
Jwe(String),
/// A rate metric. See [`RateMetric`] for more information.
Rate(i32, i32),
/// TODO
Copy link
Member

Choose a reason for hiding this comment

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

Still TODO? (I also am curious why this isn't in alphabetic order, but since it has been that way for some time... meh)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly this can't be in alphabetical order, because the order of these enum items matter when serializing/deserializing. If we change the order we might end up breaking loading up old data files. See line 88.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, makes sense. When reviewing this through the GitHub UI, it had masked line 88 so I didn't see that.

@Dexterp37 Dexterp37 requested a review from travis79 September 7, 2021 14:43
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! No need to re-review as you commit that last change (thanks GitHub UI 😆 )

@Dexterp37 Dexterp37 merged commit 1d66693 into mozilla:main Sep 7, 2021
@Dexterp37 Dexterp37 deleted the url-type branch September 7, 2021 15:49
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