Skip to content

Commit

Permalink
Allow unit only for the quantity metric
Browse files Browse the repository at this point in the history
This is implemented the same way time_unit is implemented for others:
The specific metric class takes the argument, not the generic one.

Because the schema enforces `unit` to be required for `quantity` we can
just `pop` it with the guarantee it's there.

The error message for other metrics if they do set `unit` is less nice:

    Metric.__init__() got an unexpected keyword argument 'unit'

But that's in line with what `time_unit` also does.
  • Loading branch information
badboy authored and chutten committed Oct 3, 2023
1 parent 78cbce9 commit 5079870
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 3 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
## Unreleased

- BREAKING CHANE: `ping` lifetime metrics on the events ping are now disallowed ([#625](https://github.com/mozilla/glean_parser/pull/625))
- Disallow `unit` field for anything but quantity ([#630](https://github.com/mozilla/glean_parser/pull/630)).
Note that this was already considered the case, now the code enforces it.

## 9.0.0

Expand Down
7 changes: 4 additions & 3 deletions glean_parser/metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ def __init__(
disabled: bool = False,
lifetime: str = "ping",
send_in_pings: Optional[List[str]] = None,
unit: Optional[str] = None,
gecko_datapoint: str = "",
no_lint: Optional[List[str]] = None,
data_sensitivity: Optional[List[str]] = None,
Expand Down Expand Up @@ -86,8 +85,6 @@ def __init__(
if send_in_pings is None:
send_in_pings = ["default"]
self.send_in_pings = send_in_pings
if unit is not None:
self.unit = unit
self.gecko_datapoint = gecko_datapoint
if no_lint is None:
no_lint = []
Expand Down Expand Up @@ -237,6 +234,10 @@ class Counter(Metric):
class Quantity(Metric):
typename = "quantity"

def __init__(self, *args, **kwargs):
self.unit = kwargs.pop("unit")
Metric.__init__(self, *args, **kwargs)


class TimeUnit(enum.Enum):
nanosecond = 0
Expand Down
15 changes: 15 additions & 0 deletions tests/test_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -1039,3 +1039,18 @@ def test_no_internal_fields_exposed():
indent=2,
)
assert expected_json == out_json


def test_unit_not_accepted_on_nonquantity():
results = parser.parse_objects(
[
util.add_required(
{
"category": {"metric": {"type": "counter", "unit": "quantillions"}},
}
),
]
)
errs = list(results)
assert len(errs) == 1
assert "got an unexpected keyword argument 'unit'" in str(errs[0])

0 comments on commit 5079870

Please sign in to comment.