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

Add support for ephemeral models #90

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PaddyAlton
Copy link

This PR fixes the dbt2looker tool such that ephemeral models do not break it (see issue #57).

It is inspired by earlier suggestions by @boludo00 and @mariana-s-fernandes , with minor modifications.

Copy link
Collaborator

@owlas owlas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking good! just a request to move the tag feature to a separate PR

node
for node in manifest.nodes.values()
if node.resource_type == 'model'
if node.resource_type == 'model' and node.config['materialized'] != 'ephemeral'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

]

if tag is None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you remove the tag feature from this PR and we can address it separately?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you mean, sorry?

This line isn't doing anything new, it's just some light refactoring for clarity (lines 46-48 previously did this filtering).

I guess I have changed the order in which things happen - previously, an error would have been raised for any model that didn't have a name attribute, whereas now (because I've put the filtering before the for-loop) only selected models (ones where the tag matches) that don't have a name attribute will throw the error, the rest will be ignored.

Can you confirm that this behavioural change is what you want me to revert?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@owlas I assumed that this is what you wanted, I've fiddled with it to remove the behavioural change, which I will include in a new PR shortly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All done - there are now three PRs in a stack:

@PaddyAlton
Copy link
Author

Hi @owlas - just checking whether my revisions following your review got lost in the churn?

There are three small PRs in a stack:

No particular rush, but thought I'd check this was on your radar.

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.

None yet

2 participants