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.

Sign up for GitHub

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

Jump to bottom

Bug 1638293: Make glinter messages more actionable #190

Merged
merged 1 commit into from
May 15, 2020
Merged

Bug 1638293: Make glinter messages more actionable #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions HISTORY.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ History
Unreleased
----------

* `glinter` messages have been improved with more details and to be more
actionable.

1.20.4 (2020-05-07)
-------------------

Expand Down
58 changes: 38 additions & 20 deletions glean_parser/lint.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

from . import metrics
from . import parser
from . import pings
from . import util


Expand All @@ -34,6 +35,20 @@ def _split_words(name: str) -> List[str]:
return re.split("[._]", name)


def _english_list(items: List[str]) -> str:
"""
Helper function to format a list [A, B, C] as "'A', 'B', or 'C'".
"""
if len(items) == 0:
return ""
elif len(items) == 1:
return "'{}'".format(items[0])
else:
return "{}, or '{}'".format(
", ".join(["'{}'".format(x) for x in items[:-1]]), items[-1]
)


def _hamming_distance(str1: str, str2: str) -> int:
"""
Count the # of differences between strings str1 and str2,
Expand Down Expand Up @@ -73,8 +88,9 @@ def check_common_prefix(
if i > 0:
common_prefix = "_".join(first[:i])
yield (
"Within category '{}', all metrics begin with prefix "
"'{}'. Remove prefixes and (possibly) rename category."
"Within category '{}', all metrics begin with prefix '{}'."
"Remove the prefixes on the metric names and (possibly) "
"rename the category."
).format(category_name, common_prefix)


Expand Down Expand Up @@ -114,40 +130,40 @@ def check_unit_in_name(
or unit_in_name == time_unit.name
):
yield (
"Suffix '{}' is redundant with time_unit. " "Only include time_unit."
).format(unit_in_name)
"Suffix '{}' is redundant with time_unit '{}'. Only include time_unit."
).format(unit_in_name, time_unit.name)
elif (
unit_in_name in TIME_UNIT_ABBREV.keys()
or unit_in_name in TIME_UNIT_ABBREV.values()
):
yield (
"Suffix '{}' doesn't match time_unit. "
"Suffix '{}' doesn't match time_unit '{}'. "
"Confirm the unit is correct and only include time_unit."
).format(unit_in_name)
).format(unit_in_name, time_unit.name)

elif memory_unit is not None:
if (
unit_in_name == MEMORY_UNIT_ABBREV.get(memory_unit.name)
or unit_in_name == memory_unit.name
):
yield (
"Suffix '{}' is redundant with memory_unit. "
"Suffix '{}' is redundant with memory_unit '{}'. "
"Only include memory_unit."
).format(unit_in_name)
).format(unit_in_name, memory_unit.name)
elif (
unit_in_name in MEMORY_UNIT_ABBREV.keys()
or unit_in_name in MEMORY_UNIT_ABBREV.values()
):
yield (
"Suffix '{}' doesn't match memory_unit. "
"Suffix '{}' doesn't match memory_unit '{}'. "
"Confirm the unit is correct and only include memory_unit."
).format(unit_in_name)
).format(unit_in_name, memory_unit.name)

elif unit is not None:
if unit_in_name == unit:
yield (
"Suffix '{}' is redundant with unit param. " "Only include unit."
).format(unit_in_name)
"Suffix '{}' is redundant with unit param '{}'. Only include unit."
).format(unit_in_name, unit)


def check_category_generic(
Expand All @@ -159,7 +175,9 @@ def check_category_generic(
GENERIC_CATEGORIES = ["metrics", "events"]

if category_name in GENERIC_CATEGORIES:
yield "Category '{}' is too generic.".format(category_name)
yield "Category '{}' is too generic. Don't use {} for category names".format(
category_name, _english_list(GENERIC_CATEGORIES)
)


def check_bug_number(
Expand All @@ -170,7 +188,8 @@ def check_bug_number(
if len(number_bugs):
yield (
"For bugs {}: "
"Bug numbers are deprecated and should be changed to full URLs."
"Bug numbers are deprecated and should be changed to full URLs. "
"For example, use 'http://bugzilla.mozilla.org/12345' instead of '12345'."
).format(", ".join(number_bugs))


Expand All @@ -182,17 +201,15 @@ def check_valid_in_baseline(
if not allow_reserved and "baseline" in metric.send_in_pings:
yield (
"The baseline ping is Glean-internal. "
"User metrics should go into the 'metrics' ping or custom pings."
"Remove 'baseline' from the send_in_pings array."
)


def check_misspelled_pings(
metric: metrics.Metric, parser_config: Dict[str, Any] = {}
) -> LintGenerator:
builtin_pings = ["metrics", "events"]

for ping in metric.send_in_pings:
for builtin in builtin_pings:
for builtin in pings.RESERVED_PING_NAMES:
distance = _hamming_distance(ping, builtin)
if distance == 1:
yield ("Ping '{}' seems misspelled. Did you mean '{}'?").format(
Expand All @@ -206,8 +223,9 @@ def check_user_lifetime_expiration(

if metric.lifetime == metrics.Lifetime.user and metric.expires != "never":
yield (
"Metrics with `user` lifetime cannot have an expiration date. "
"They live as long as the user profile does."
"Metrics with 'user' lifetime cannot have an expiration date. "
"They live as long as the user profile does. "
"Set expires to 'never'."
)


Expand Down