Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix rule E7 in the case of refinements #163

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

arthurbellis
Copy link
Contributor

Hi @fabio-looker, I hope you're doing well. I've got another refinement-related contribution, hopefully it's helpful this time. :-)

This fixes an issue with E7 applied to explores that have refinements. After some exploration, I found ignoring the refinements in the model and only looking at the final explore in the model (since the refinements are applied) seemed a like a good fix.

Test dummy-projects/20-refinements/e7 makes sure that a label added on a long-named explore in a refinement is handled correctly.

Before the fix, I see this error:

 FAIL  __tests__/dummy-projects/20-refinements/e7/index.test.js
 [...]
        {"description": "Error evaluating rule: Cannot read properties of undefined (reading 'length')", "level": "error", "location": "model:ok/explore:+datawarehouse_bigquery__schema_business__datamart_ecommerce__facts__orders", "rule": "E7"}

This appears to be because /rules/e7.js assumes explore is a dict, but it might be an array of refinements.

After the fix in commit 4aacd27, the new test passes.

@fabio-looker
Copy link
Contributor

Thanks for including the test case with this, it's very helpful!

I'll take a look at the implementation to doublecheck what's going on here, but my vague recollection is that the parser should be returning these already fully resolved within the model object, which should prevent the need for these kinds of guards, but I will confirm to be sure.

@arthurbellis
Copy link
Contributor Author

I'll take a look at the implementation to doublecheck what's going on here, but my vague recollection is that the parser should be returning these already fully resolved within the model object

Thanks, I tried to understand exactly what the parser was doing, but very likely there is a more elegant solution.

The parser results do include the fully-resolved explore as-is, with refinements applied, but it also includes an explore with name +explore_name and an array of the refinements. So this change just ignores the latter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants