Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 basic dbt objects and rule definitions #6
Add basic dbt objects and rule definitions #6
Changes from 20 commits
794e79b
25d1059
37c0a01
8811c28
4a620e8
d1ddd4b
5e4a4bc
7e24cdd
e1e0fb3
787bebf
dce673b
c14d7a7
b8636f2
db2643b
66e2c73
cb82aa2
2bf66c5
8256171
05c46ac
c77279a
cbca0b2
23282db
09b0883
2a4966c
a21c5c4
8d94bf9
f396137
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure you've thought a lot about this setup and I'd also like to learn more so here goes:
What is the reason
@rule
returns a subclass of Rule, rather than a Rule object? That is, instead of:Why not:
Generating subclasses, of which there will only be one object per subclass, feels similar to generating objects, i.e. specific instances of
Rule
, but the latter is semantically closer to what you'd want (I think).rule
would then look something like this:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the reasons is to allow users to create more advanced rules by subclassing
Rule
(simpler rules are encouraged to use the diet@rule
decorator). Does that make sense?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is indeed very similar, but this way it will be easier for users to create their own complex rules with a class. So this gives full flexibility for users that need it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But can't they still do that even in this setup?
Then in the rule registry / runner:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snippet above is how it currently works. But with your suggestion of creating instances instead of classes in the decorator,
some_simple_rule
would already be an instance, thereforesome_simple_rule()
would not work, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I missed the last
return
🙈Which should result in:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. But I think it's more straightforward to go with the approach: "every subclass of
Rule
is a rule". Such subclass can be created either by subclassing, or by using@rule
as syntactic sugar. This puts all rules "at the same level", if that makes sense?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I see it, both approaches are the same in the sense that "every
Rule
is a rule".I understand that, by always subclassing
Rule
, all rules are on the same hierarchy. But from another perspective, they're allRule
s and in aggregate we will work with lists ofRule
s. It doesn't matter if you subclass aRule
or use a subclass of a subclass ofRule
or useRule
itself.From a user perspective, nothing will change. You either use
@rule
as syntactic sugar or subclassRule
.Where they differ is that I think, semantically, a list of objects is more straightforward than a list of generated subclasses. And I also think it simplifies implementation a bit.
Btw, because nothing changes from a user's perspective, I wouldn't make this point blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have given it another look and decided to go with the current implementation. Going with the instance implementation also has some challenges, e.g. the example
will not exactly work. To call
some_simple_rule()
we will need to grab thefunc
from somewhere again as it's an input variable. I think the implementation does not necessarily becomes easier when using instances instead of classes.