Improve and simplify API and impl of action_utils [chromium/src : main]
Alexei Svitkine (Gerrit)
Alexei Svitkine voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alexei Svitkine
| Code-Review | +1 |
1 comment
token: a Token object to get the key from.Add pydoc for the return.
Related details
- Alicja Opalinska
- Andrzej Fiedukowicz
- Luc Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine added 1 comment![]( "Open in Gerrit")
Andrzej Fiedukowicz (Gerrit)
Andrzej Fiedukowicz added 2 comments![]( "Open in Gerrit")
Add pydoc for the return.
- Alexei Svitkine
- Alicja Opalinska
- Luc Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luc Nguyen (Gerrit)
Luc Nguyen added 6 comments![]( "Open in Gerrit")
actions_dict: dict of existing action name to Action object.
action: an Action object to combine with suffix.
variant: a Variant object to combine with action.
token: a Token object to get the key from.nit: update this
actions_dict: A dict of existing action name to Action object.
variants_dict: A dict of variants name to list of Variant objects.nit: update this
expanded_actions[action.name] = actionnit: add `continue` just to be explicit that there's no other work to do
for token in action.tokens:
for variant in token.variants:
new_action = _CreateActionFromVariant(action, variant, token)
expanded_actions[new_action.name] = new_actionIIUC this doesn't work if the action has multiple variants right? e.g. `Action{Token1}{Token2}`. do we intentionally not support them? i don't see any TODOs and couldn't really find anything in our buglist, and https://chromium.googlesource.com/chromium/src/+/master/tools/metrics/histograms/README.md is pretty lackluster in terms of variants support
(non-blocking because the previous code is the same)
logging.error(
f'The variants attribute {variants_name} of token key {token_key} '
f'of action {action_name} does not have a corresponding '
f'<variants> tag.')should this be fatal? since we're now accessing `variants_dict[variants_name]` below
return extract_actions.PrettyPrint(actions_dict | expanded_actions,nit: is the `|` necessary? i see in `CreateActionsFromVariants()` that we leave the actions that don't need expanding untouched:
```python
# If there are no tokens to fill, the action goes to the list as-is.
if not action.tokens:
expanded_actions[action.name] = action
```
Related details
- Alexei Svitkine
- Alicja Opalinska
- Andrzej Fiedukowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luc Nguyen (Gerrit)
Luc Nguyen added 1 comment![]( "Open in Gerrit")
for token in action.tokens:
for variant in token.variants:
new_action = _CreateActionFromVariant(action, variant, token)
expanded_actions[new_action.name] = new_actionIIUC this doesn't work if the action has multiple variants right? e.g. `Action{Token1}{Token2}`. do we intentionally not support them? i don't see any TODOs and couldn't really find anything in our buglist, and https://chromium.googlesource.com/chromium/src/+/master/tools/metrics/histograms/README.md is pretty lackluster in terms of variants support
(non-blocking because the previous code is the same)
hmm, nvm, i just saw https://chromium-review.googlesource.com/c/chromium/src/+/7277276 :-)
Luc Nguyen (Gerrit)
Luc Nguyen added 1 comment![]( "Open in Gerrit")
variants_name = f"TypeOf{token.key}"nit: what's the purpose of this "TypeOf" prefix?
Andrzej Fiedukowicz (Gerrit)
Andrzej Fiedukowicz added 7 comments![]( "Open in Gerrit")
actions_dict: dict of existing action name to Action object.
action: an Action object to combine with suffix.
variant: a Variant object to combine with action.
token: a Token object to get the key from.Andrzej Fiedukowicznit: update this
Done
actions_dict: A dict of existing action name to Action object.
variants_dict: A dict of variants name to list of Variant objects.Andrzej Fiedukowicznit: update this
Done
nit: add `continue` just to be explicit that there's no other work to do
Done
for token in action.tokens:
for variant in token.variants:
new_action = _CreateActionFromVariant(action, variant, token)
expanded_actions[new_action.name] = new_actionLuc NguyenIIUC this doesn't work if the action has multiple variants right? e.g. `Action{Token1}{Token2}`. do we intentionally not support them? i don't see any TODOs and couldn't really find anything in our buglist, and https://chromium.googlesource.com/chromium/src/+/master/tools/metrics/histograms/README.md is pretty lackluster in terms of variants support
(non-blocking because the previous code is the same)
hmm, nvm, i just saw https://chromium-review.googlesource.com/c/chromium/src/+/7277276 :-)
Yeah, and we actually do support and even have multiple of those already, it was just not added to this utils code leading to unexpected results 😊
nit: what's the purpose of this "TypeOf" prefix?
To not pass the test when the strings gets crossed and we pass Token name as its type or the other way around.
logging.error(
f'The variants attribute {variants_name} of token key {token_key} '
f'of action {action_name} does not have a corresponding '
f'<variants> tag.')should this be fatal? since we're now accessing `variants_dict[variants_name]` below
Good point, done!
return extract_actions.PrettyPrint(actions_dict | expanded_actions,nit: is the `|` necessary? i see in `CreateActionsFromVariants()` that we leave the actions that don't need expanding untouched:
```python
# If there are no tokens to fill, the action goes to the list as-is.
if not action.tokens:
expanded_actions[action.name] = action
```
It's for backward compatibility, as the test wants the unexpanded tokens to be in the output. See: BASIC_VARIANT_EXPANDED_XML.
I think it's a test that doesn't test anything real though. I will review those tests in separate CLs to not mix up to many things here.
Related details
- Alexei Svitkine
- Alicja Opalinska
- Luc Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luc Nguyen (Gerrit)
Luc Nguyen voted and added 3 comments![]( "Open in Gerrit")
Votes added by Luc Nguyen
| Code-Review | +1 |
3 comments
variants_name = f"TypeOf{token.key}"Andrzej Fiedukowicznit: what's the purpose of this "TypeOf" prefix?
To not pass the test when the strings gets crossed and we pass Token name as its type or the other way around.
can you add a comment?
logging.error(
f'The variants attribute {variants_name} of token key {token_key} '
f'of action {action_name} does not have a corresponding '
f'<variants> tag.')Andrzej Fiedukowiczshould this be fatal? since we're now accessing `variants_dict[variants_name]` below
Good point, done!
sorry, i said fatal, but i meant more generally that the script shouldn't proceed further down -- e.g. i see that the other errors above simply `continue`
return extract_actions.PrettyPrint(actions_dict | expanded_actions,Andrzej Fiedukowicznit: is the `|` necessary? i see in `CreateActionsFromVariants()` that we leave the actions that don't need expanding untouched:
```python
# If there are no tokens to fill, the action goes to the list as-is.
if not action.tokens:
expanded_actions[action.name] = action
```
It's for backward compatibility, as the test wants the unexpanded tokens to be in the output. See: BASIC_VARIANT_EXPANDED_XML.
I think it's a test that doesn't test anything real though. I will review those tests in separate CLs to not mix up to many things here.
- Alexei Svitkine
- Alicja Opalinska
- Andrzej Fiedukowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrzej Fiedukowicz (Gerrit)
Andrzej Fiedukowicz added 2 comments![]( "Open in Gerrit")
variants_name = f"TypeOf{token.key}"Andrzej Fiedukowicznit: what's the purpose of this "TypeOf" prefix?
Luc NguyenTo not pass the test when the strings gets crossed and we pass Token name as its type or the other way around.
can you add a comment?
Done
logging.error(
f'The variants attribute {variants_name} of token key {token_key} '
f'of action {action_name} does not have a corresponding '
f'<variants> tag.')Andrzej Fiedukowiczshould this be fatal? since we're now accessing `variants_dict[variants_name]` below
Luc NguyenGood point, done!
sorry, i said fatal, but i meant more generally that the script shouldn't proceed further down -- e.g. i see that the other errors above simply `continue`
I actually replace it in later CL with exception, so I hope it's fine for now to simplify things.
Related details
- Alexei Svitkine
- Alicja Opalinska
- Luc Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luc Nguyen (Gerrit)
Luc Nguyen voted Code-Review+1![]( "Open in Gerrit")
- Alexei Svitkine
- Alicja Opalinska
- Andrzej Fiedukowicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrzej Fiedukowicz (Gerrit)
Andrzej Fiedukowicz added 1 comment![]( "Open in Gerrit")
Related details
- Alexei Svitkine
- Alicja Opalinska
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexei Svitkine (Gerrit)
Alexei Svitkine voted![]( "Open in Gerrit")
- Alicja Opalinska
- Andrzej Fiedukowicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Improve and simplify API and impl of action_utils
The API of action_utils was error prone with in place modification
of the passed object. Instead it got refactored to operate by return
value making it clear which objects are expected in it.
Additionally the logic was simplified significantly and number of
arguments to functions reduced.
This still doesn't work for more then one token in action, but this
will be fixed in the next CL.
TESTED=Newly added tests are passing
- M tools/metrics/actions/action_utils.py
- A tools/metrics/actions/action_utils_test.py
- M tools/metrics/actions/extract_actions.py
- M tools/metrics/actions/extract_actions_test.py
Code-Review: +1 by Alexei Svitkine, +1 by Luc Nguyen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |