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

Create rule registry and discovery #5

Merged
merged 3 commits into from
Mar 29, 2024
Merged

Conversation

matthieucan
Copy link
Contributor

The rule registry has 2 purposes:

  • be a container for all known rules
  • discover rules in a given namespace

By default, the rule registry looks for rules in 2 places:

  • dbt_score.rules, the "core" rules of dbt-score
  • dbt_score_rules, a namespace that anyone can use to add custom rules

@jochemvandooren
Copy link
Contributor

Before diving into the code: could you maybe add a some documentation to explain how rule registry works in our package? We will need the documentation anyway, and it will help our reviewing efforts!

@matthieucan
Copy link
Contributor Author

Before diving into the code: could you maybe add a some documentation to explain how rule registry works in our package? We will need the documentation anyway, and it will help our reviewing efforts!

Can you let me know what you are lacking? Hopefully the docstrings are already helpful.
When it comes to user documentation about the possibilities to add new rules, we'll certainly get there, but given the mechanism is not fully in place yet (i.e. no @rule implementation), I think this should come later.

@jochemvandooren
Copy link
Contributor

Before diving into the code: could you maybe add a some documentation to explain how rule registry works in our package? We will need the documentation anyway, and it will help our reviewing efforts!

Can you let me know what you are lacking? Hopefully the docstrings are already helpful. When it comes to user documentation about the possibilities to add new rules, we'll certainly get there, but given the mechanism is not fully in place yet (i.e. no @rule implementation), I think this should come later.

Yeah I should be able to understand it. Makes sense to write the docs when it is set in stone, I will use the docstrings etc. 👍

Copy link
Contributor

@jochemvandooren jochemvandooren left a comment

Choose a reason for hiding this comment

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

Looks good, works very well locally! 🤩 Just left a few comments with some questions. Also you can make the changes to comply with the new Rule definitions that were merged into master.

src/dbt_score/definitions.py Outdated Show resolved Hide resolved
src/dbt_score/registry.py Outdated Show resolved Hide resolved

from dbt_score.exceptions import DuplicatedRuleException

THIRD_PARTY_RULES_NAMESPACE = "dbt_score_rules"
Copy link
Contributor

Choose a reason for hiding this comment

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

I am trying to understand the expected behavior. Is it correct that this folder should be in the root of the project where users want to run dbt-score? As I understand, this folder can not live for example in a subdirectory. Not sure if this makes sense, we can discuss

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't assume any folder structure, but the presence of the namespace dbt_score_rules in the Python path (which can live anywhere, as long as it's known by Python at run time). Does that make sense?

Copy link
Contributor

@druzhinin-kirill druzhinin-kirill left a comment

Choose a reason for hiding this comment

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

💅

tests/rules/example.py Outdated Show resolved Hide resolved
tests/rules/nested/example.py Outdated Show resolved Hide resolved
Copy link
Contributor

@druzhinin-kirill druzhinin-kirill left a comment

Choose a reason for hiding this comment

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

💪
Left a minor comment

src/dbt_score/rule_registry.py Show resolved Hide resolved
Copy link
Contributor

@jochemvandooren jochemvandooren left a comment

Choose a reason for hiding this comment

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

🚀

@matthieucan matthieucan merged commit 8296976 into master Mar 29, 2024
2 checks passed
@matthieucan matthieucan deleted the matthieucan/rule-discovery branch March 29, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants