Add is_array filter and test for AttributeType

[arthurDiff]

Added is_array fn to AttributeType enum.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: arthurDiff / name: Arthur Lee (f61d62a, 7860ebc, 545aa8d, bc446ae, 2e48618, ed60aa5)
• ✅ login: lquerel / name: Laurent Quérel (02f4eef)

[lquerel]

@arthurDiff Thank you for this PR, but I believe this ticket unfortunately lacked detail. What @lmolkova originally wanted was a Jinja filter or test integrated into our template engine based on the Minijinja implementation.

The idea would be to replace this Jinja hack (see the original pb description here):

{%- if "[]" in attribute.type and "template" not in attribute.type %}
...

with a Jinja test similar to this declaration:

{%- if attribute is array %}
...

You can find several examples of such tests in the following file (the one you could update):

crates/weaver_forge/src/extensions/otel.rs

I would be happy to provide more details or any kind of help if needed.

[arthurDiff]

@arthurDiff Thank you for this PR, but I believe this ticket unfortunately lacked detail. What @lmolkova originally wanted was a Jinja filter or test integrated into our template engine based on the Minijinja implementation.

The idea would be to replace this Jinja hack (see the original pb description here):

{%- if "[]" in attribute.type and "template" not in attribute.type %}
...

with a Jinja test similar to this declaration:

{%- if attribute is array %}
...

You can find several examples of such tests in the following file (the one you could update):

crates/weaver_forge/src/extensions/otel.rs

I would be happy to provide more details or any kind of help if needed.

Hi @lquerel ! Sorry about the delay and thanks for the clarification! Should be implemented with minijinja now.

[lquerel]

@lmolkova This test looks fine to me, but could you confirm: for semconvs, should a template[xxx[]] be considered an array, or should we treat any template as not being an array, even when templating an array of something?

[arthurDiff]

Just added in documentation update and new test case for full coverage! just let me know if that needs updating!

[lquerel]

Overall LGTM. Just 2 comments:

• I asked Liudmila to confirm if templates of arrays must be considered arrays. Nothing to do on your side for now.
• You should add a description of the Jinja test in the following documentation.

After we should be ready to go :-).

Thank you

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.5%. Comparing base (213fa72) to head (02f4eef).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/weaver_forge/src/extensions/otel.rs	80.0%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main    #540   +/-   ##
=====================================
  Coverage   74.5%   74.5%           
=====================================
  Files         51      51           
  Lines       3960    3965    +5     
=====================================
+ Hits        2953    2957    +4     
- Misses      1007    1008    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[arthurDiff]

Overall LGTM. Just 2 comments:

• I asked Liudmila to confirm if templates of arrays must be considered arrays. Nothing to do on your side for now.
• You should add a description of the Jinja test in the following documentation.

After we should be ready to go :-).

Thank you

Sounds good! Out of curiosity. is there reason for match attribute types; weaver uses literal string instead of static const?
For example like this?

impl AttributeType {
    const PRIMITIVE_STRING: &str = "string";
    const PRIMITIVE_STRING_ARRAY: &str = "string[]";
}

or 

impl PrimitiveOrArrayTypeSpec{
    const STR: &str = "string";
    const STR_ARRAY: &str = "string[]";
}

[lquerel]

@arthurDiff Regarding your question about strings, I’m not sure I fully understand. Are you suggesting introducing constants instead of using literal strings directly in the code? If so, then yes, they could indeed be replaced with static const &str.

Additionally, I didn’t understand why you closed this PR after making changes. It’s probably an error, so I'm reopening it so that we can proceed with the merge once we have received a response from @lmolkova. Thanks

[arthurDiff]

@arthurDiff Regarding your question about strings, I’m not sure I fully understand. Are you suggesting introducing constants instead of using literal strings directly in the code? If so, then yes, they could indeed be replaced with static const &str.

Additionally, I didn’t understand why you closed this PR after making changes. It’s probably an error, so I'm reopening it so that we can proceed with the merge once we have received a response from @lmolkova. Thanks

Yup! I think using static const str will help avoid errors instead of string literal.
and sorry about that rushing this morning must have closed it by accident! thanks for reopening it.
I will wait for @lmolkova and get it merged with response! Thanks for the information.