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

Support configuration using pyproject.toml #16

Merged
merged 30 commits into from
May 14, 2024

Conversation

jochemvandooren
Copy link
Contributor

@jochemvandooren jochemvandooren commented May 1, 2024

Support general configuration and rule configuration stored in pyproject.toml.

  • Rules are now identified by their full name, including namespace.
  • The default namespace(s) can be configured.
  • Rules can now be disabled and configured.

@jochemvandooren jochemvandooren self-assigned this May 1, 2024
@jochemvandooren jochemvandooren marked this pull request as ready for review May 1, 2024 12:30
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.

👍

src/dbt_score/rule.py Show resolved Hide resolved
src/dbt_score/rule.py Outdated Show resolved Hide resolved
src/dbt_score/config_parser.py Outdated Show resolved Hide resolved
src/dbt_score/config_parser.py Outdated Show resolved Hide resolved
src/dbt_score/lint.py Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/dbt_score/config_parser.py Outdated Show resolved Hide resolved
src/dbt_score/config_parser.py Outdated Show resolved Hide resolved
src/dbt_score/config_parser.py Outdated Show resolved Hide resolved
src/dbt_score/config_parser.py Outdated Show resolved Hide resolved
src/dbt_score/lint.py Outdated Show resolved Hide resolved
src/dbt_score/rule.py Outdated Show resolved Hide resolved
src/dbt_score/rule.py Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
src/dbt_score/cli.py Outdated Show resolved Hide resolved
src/dbt_score/config.py Show resolved Hide resolved
src/dbt_score/config.py Outdated Show resolved Hide resolved
src/dbt_score/config.py Show resolved Hide resolved
src/dbt_score/config.py Outdated Show resolved Hide resolved
src/dbt_score/config.py Outdated Show resolved Hide resolved
src/dbt_score/rule.py Outdated Show resolved Hide resolved
src/dbt_score/rule_registry.py Outdated Show resolved Hide resolved
"""Configuration for a rule."""

severity: int | None = None
params: dict[str, Any] = field(default_factory=dict)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be named config (also in Rule.params -> Rule.config)?
Reason is, I find it confusing to have these 2 concepts, which are fundamentally the same but named differently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this actually prevents confusion because these things are not the same IMO. Yes both are to configure a Rule, but one configuration is input for a method whereas the other also does other things (update severity). I am open to change it, maybe @druzhinin-kirill has an opinion on what is less confusing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Rule.config_params 😄

No strong opinion, however, I would prefer avoiding key mismatch variable names:

return RuleConfig(severity=severity, params=rule_config)

so either variable is params, or the argument is rules (only if it's achievable for zero cost, super minor anyway).

Copy link
Contributor

@matthieucan matthieucan May 10, 2024

Choose a reason for hiding this comment

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

From the user point of view, this is all "rule config", right?
Now internally the config is divided in two, severity and params/config. I don't think it's an issue, we could do something like

return RuleConfig(severity=severity, config=config)

which I believe is self explanatory, and doesn't require the use of multiple terms? WDYT?

Copy link
Contributor Author

@jochemvandooren jochemvandooren May 13, 2024

Choose a reason for hiding this comment

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

From the user point of view it's indeed all the same, it's internals only.

Ok I will go for using config inside RuleConfig 👌

src/dbt_score/config.py Outdated Show resolved Hide resolved
src/dbt_score/config.py Outdated Show resolved Hide resolved
tests/test_cli.py Outdated Show resolved Hide resolved
Copy link
Contributor

@matthieucan matthieucan left a comment

Choose a reason for hiding this comment

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

🚀

@jochemvandooren jochemvandooren merged commit 4afbe50 into master May 14, 2024
2 checks passed
@jochemvandooren jochemvandooren deleted the jvandooren/rule-configuration branch May 14, 2024 10:13
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