- Notifications
You must be signed in to change notification settings - Fork102
chore: expose schema_cache & file_context in lint rules#449
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
any reason why we do not simply put these into the rule context? from an API standpoint, I feel like they should?
| ctx: &RuleContext<Self>, | ||
| _file_context: &AnalysedFileContext, | ||
| _schema_cache: Option<&SchemaCache>, |
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.
from an API standpoint, I would put these two into theRuleContext. After all, they are "context" info.
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 felt theRuleContext held static metadata about the rule, while the other stuff is related to the db/data model and the parsed sql file.
But I don't have a hard opinion
| @@ -0,0 +1,7 @@ | |||
| #[derive(Default)] | |||
| pub struct AnalysedFileContext {} | |||
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.
what is this for?
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.
That's just a scaffold for the file context.
We can add properties in future linting PRs.
eugene works the same way:link
c323eb0 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Analyser::runnow takesVec<AnalysableStatement>and anOption<&SchemaCache>AnalysedFileContextSchemaCacheand theAnalysedFileContextare passed into each linting rule.This should roughly match how theeugene code base does it.
The rest of the changes are adaptions to those changes or minor things, e.g. a new document-mapper or some renamings.