Add the accumulate filter #9133
Conversation
VannTen
force-pushed
the
feat/filter_accumulate
branch
from
9f5ec31
to
2cc84b9
Compare
ansibullbot
added
integration
This comment was marked as outdated.
This comment was marked as outdated.
ansibullbot
added
ci_verified
VannTen
force-pushed
the
feat/filter_accumulate
branch
from
2cc84b9
to
a6a7dd0
Compare
- Add myself as a maintainer for it. - Some integration tests.
VannTen
force-pushed
the
feat/filter_accumulate
branch
from
a6a7dd0
to
7c5e7bf
Compare
ansibullbot
removed
ci_verified
felixfontein
added
check-before-release
|
I was not sure of what to put in version_added so I put the same that the first entry I could find by doing `git log -S version_added`, if that's incorrect and this new filter is included, please advise on what I should put here, thanks
|
The aliases file was copied over from tests/integrations/targets/filter_dict/aliases as the documentation[1] suggests to use the same group as existing similar tests. [1]: https://docs.ansible.com/ansible/latest/dev_guide/testing/sanity/integration-aliases.html Suggested-by: Felix Fontein <[email protected]>
|
Other than the small adjustment in the markup, LGTM |
Suggested-by: Felix Fontein <[email protected]> Suggested-by: Alexei Znamensky <[email protected]>
|
|
||
| def filters(self): | ||
| return { | ||
| 'accumulate': list_accumulate, |
There was a problem hiding this comment.
Why not
| 'accumulate': list_accumulate, | |
| 'accumulate': accumulate, |
?
list_accumulate is not really doing anything.
There was a problem hiding this comment.
It is doing something: it's allowing exactly one positional argument, thus restricting the arguments to what is documented.
(It also allows to add better error checking at some point.)
There was a problem hiding this comment.
Yeah I had considered whether to forward more arguments to python accumulate, but:
- I don't think there is an ergonomic way to pass a binary function
- Passing
initialwould be roughly equivalent to([initial_elem] + input_list) | accumulate)so I don't think it's worth adding an optional argument.
(It also allows to add better error checking at some point.)
What do you have in mind ? I could just as well add it now.
There was a problem hiding this comment.
Somethings like checking for args and kward and raising an exception in there is some ?
There was a problem hiding this comment.
What do you have in mind ? I could just as well add it now.
Checking whether the sequence is actually a sequence.
With from collections.abc import Sequence you can use if not isinstance(sequence, Sequence): to test it.
(This also accepts strings as sequences. That's generally not so great, but here strings are also OK, I guess.)
There was a problem hiding this comment.
Do you have time to work on this before say in a week?
There was a problem hiding this comment.
Thinking back on this, I'm wondering if I should add that. There is already some quite readable errors message coming directly from accumulate, for instance I I tried to feed an integer to the implementation as-is:
TASK [filter_accumulate : Test accumulate filter] ******************************
fatal: [testhost]: FAILED! => {"msg": "The conditional check '1 | community.general.accumulate == 1' failed. The error was: Unexpected templating type error occurred on ({% if 1 | community.general.accumulate == 1 %} True {% else %} False {% endif %}): 'int' object is not iterable. 'int' object is not iterable"}
However, the filter accepts some input which, while they might be useful, could be questionnable from a correctness POV.
TASK [filter_accumulate : debug] ***********************************************
ok: [testhost] => {
"{'test': 1, 'ee': 1} | community.general.accumulate": [
"test",
"testee"
]
}
-> this one errors when checking for a Sequence type.
Wdyt ? I would tend to got the more permissive way, and document the unobvious cases, but I don't have strong opinions.
There was a problem hiding this comment.
Also, it appears that checking for Sequence does not catch subtler invalid case, such as non compatible list members :
TASK [filter_accumulate : Test accumulate filter] ******************************
fatal: [testhost]: FAILED! => {"msg": "The conditional check '['ac', 1] | community.general.accumulate == ['ac', 'ac1']' failed. The error was: Unexpected templating type error occurred on ({% if ['ac', 1] | community.general.accumulate == ['ac', 'ac1'] %} True {% else %} False {% endif %}): can only concatenate str (not \"int\") to str. can only concatenate str (not \"int\") to str"}
There was a problem hiding this comment.
-> this one errors when checking for a Sequence type.
If you really want to do that, you can always use the | list filter to convert the dict to a list (of its keys) first.
Also, it appears that checking for Sequence does not catch subtler invalid case, such as non compatible list members :
Obviously it cannot catch all invalid input, but it can catch some of the more obvious ones.
Wdyt ? I would tend to got the more permissive way, and document the unobvious cases, but I don't have strong opinions.
I would definitely check for Sequence.
|
Yeah, I should be able to add that at the start of next week.
This one was rather full ^
|
Accepting arbitrary iterables might lead to surprising behavior so we are stricter on what we accept in the filter. Relaxing those requirements is easier than retrofitting them, in terms of backwards compatibility. Suggested-by: Felix Fontein <[email protected]> Signed-off-by: Max Gautier <[email protected]>
Signed-off-by: Max Gautier <[email protected]>
SUMMARY
Add the accumulate filter, a passthrough to python itertools.accumulate
See the below issue for motivation.
Fixes #9129
ISSUE TYPE
COMPONENT NAME
filter/accumulate
ADDITIONAL INFORMATION
N/A