Applying linter rules to views that are extends is too restrictive

[ephraimsd]

The conditions to exempt a view from Primary Key Dimension rules K1-K4 basically amount to: If a view is not an extends and does not have either sql_table_name or derived_table, then exempt it from rules K1-K4.

This means if view_1 has a primary key defined, and view_2 is defined as an extends of view_1, then view_2 will be (incorrectly) flagged by the linter as not having a primary key defined (even though it inherits the primary key from view_1).

Shouldn't it be enough for the linter to enforce K1-K4 rule compliance on views that have a sql_table_name or a derived_table?

look-at-me-sideways/rules/k1-2-3-4.js

Line 55 in 02aa750

if (!view.derived_table && !view.sql_table_name && !view.extends) {

[fabio-looker]

Hi @ephraimsd

This is actually great timing to talk about this, as I am currently working on a feature to have the linter evaluate rules based on objects as refined/extended, rather than as written locally.

Hopefully, your concern will be resolved by that work. However, I want to make sure I understand it, since I am not sure I am following the concern.

The main reason why I am skipping over views that have extends (meaning they extend from some other base view) is that developers might comply with a rule in a number of places, and failing an extending view for lack of certain fields seemed too "harsh". Without skipping extending views, tenant_1_view here would error:

view: base_view {
  dimension: pk1_id {}
  # ... other standard/base fields
}
view: tenant_1_view {
  extends: [base_view]
  sql_table_name: tenant_1_table ;;
  dimension: tenant_1_extra_calculation {...}
}

[ephraimsd]

Hi @fabio-looker , thank you for your quick response. From my reading of the code, it seems to me that tenant_1_view will actually not be skipped when checking the rules, since it will fail !view.extends.

For example, in our project we have interaction_author_user, which extends user_dim:

view: interaction_author_user {
  extends: [user_dim]

view: user_dim {
  sql_table_name: "LOOKER_REPORTING"."USER" ;;

  dimension: pk1_user_id {
    hidden: yes
    primary_key: yes
    sql: ${TABLE}."USER_ID" ;;
    type: string
  }

However, even though user_dim has a primary_key defined, we get the following linter error for interaction_author_user in our issues.md file:

[fabio-looker]

You're right! I misread my own code here.

And you're also right that trying to enforce the rule against a view that has extends is of little value and often/usually creates unnecessary problems.

That said, let me put this issue on pause until next week when hopefully I will release the enhancement to allow the linter to work on extended/refined objects. I already have the changes in the parser done, and now it's a matter of updating rule implementations.

Edit: and if by next week anything comes up to prevent that release, I will go ahead and fix this conditional in a patch release

[ephraimsd]

Thank you so much @fabio-looker !

[fabio-looker]

Sorry this took me so long to get to. v2.1.5 should make this condition much more permissive. Also, I am still looking to revamp handling of extends in v3 once I get to it...

[ephraimsd]

Thank you @fabio-looker !