looker-open-source / look-at-me-sideways

A style guide and linter for Looker's LookML data modeling language
MIT License
123 stars 37 forks source link

F1 bug: doesn't filter _value properly #167

Closed erictt closed 2 months ago

erictt commented 5 months ago

For example,

    html:
    {% if currency._value == "usd" %}
    <p>${{rendered_value}}</p>
    {% elsif currency._value == "gbp" %}
    <p>£{{rendered_value}}</p>
    {% elsif currency._value == "eur" %}
    <p>€{{rendered_value}}</p>
    {% else %}
    <p>{{rendered_value}}</p>
    {% endif %}
    ;;
    description: "Amount"

This will trigger the F1 error.

❌      F1..... v:capital_advance_funnel/d:amount.............. amount references another view, if currency,  via if currency._value == "usd" %

I checked the regex here, and found the parts actually are:

[ 'if currency', '_value == "usd" %' ]

Given the parts, the filter list won't work since _value is not identified as a single field.

erictt commented 5 months ago

Maybe use a split at first before checking the exact match?

parts[parts.length - 1].split(" ").some(substring => [
            'SQL_TABLE_NAME',
            '_sql',
            '_value',
            '_name',
            '_filters',
            '_parameter_value',
            '_rendered_value',
            '_label',
            '_link',
        ].includes(substring));

for this: https://github.com/looker-open-source/look-at-me-sideways/blob/master/rules/f1.js#L60

fabio-looker commented 5 months ago

Yea, that regex is definitely suspect! Thanks for the clear example, I'll queue this up :)

mariana-s-fernandes commented 4 months ago

I have the same issue with fields dependent on parameters

fabio-looker commented 2 months ago

@erictt's PR should be published to v3.1.3 and v2.1.8

@mariana-s-fernandes - does Eric's fix address your comment too, or no?

erictt commented 2 months ago

@erictt's PR should be published to v3.1.3 and v2.1.8

@mariana-s-fernandes - does Eric's fix address your comment too, or no?

Thank you so much for the merging!

fabio-looker commented 2 months ago

Thank you so much for the merging!

My pleasure! I was a little strapped for time in the second half of Q1, so my apologies it took me so long. Had a few things to clean up & add along with the merge and it took me a while to get to it

mariana-s-fernandes commented 2 months ago

@fabio-looker this one is ok now! Thanks! But my linting result is completely different now... But I will raise it somewhere else