Fix missing arguments to WorkflowLinter
#2809
Conversation
JCZuurmond
added
integration test failure
internal
JCZuurmond
had a problem deploying
to
account-admin
GitHub Actions
Failure
| return WorkflowLinter( | ||
| self.workspace_client, | ||
| self.dependency_resolver, | ||
| self.path_lookup, | ||
| TableMigrationIndex([]), # TODO: bring back self.tables_migrator.index() | ||
| self.directfs_access_crawler_for_paths, | ||
| self.used_tables_crawler_for_paths, |
There was a problem hiding this comment.
@ericvergnaud : Is the right UsedTablescrawler?
|
Failing integration tests will be covered in other PRs, see integration test failure issues. |
|
Want to reuse tests from #2788 to increase the test coverage which blocks the CI |
JCZuurmond
force-pushed
the
fix/workflow-linter-on-context
branch
from
bb63a48 to
b3590b5
Compare
JCZuurmond
had a problem deploying
to
account-admin
GitHub Actions
Error
JCZuurmond
had a problem deploying
to
account-admin
GitHub Actions
Failure
|
✅ 42/42 passed, 1 flaky, 42m53s total Flaky tests:
Running from acceptance #6352 |
JCZuurmond
had a problem deploying
to
account-admin
GitHub Actions
Error
JCZuurmond
temporarily deployed
to
account-admin
GitHub Actions
Inactive
| return UsedTablesCrawler(backend, schema, "used_tables_in_queries") | ||
|
|
||
| def __init__(self, backend: SqlBackend, schema: str, table: str): | ||
| def __init__(self, backend: SqlBackend, schema: str, table: str) -> None: |
There was a problem hiding this comment.
That's bad. For clarity class factory methods should always come before instance methods
There was a problem hiding this comment.
It is bad nor good, it is a style issue, this is consistent with the code base
| ) | ||
| def test_global_context_attributes_not_none(attribute: str): | ||
| def test_global_context_attributes_not_none(attribute: str) -> None: | ||
| """Attributes should be not None""" |
There was a problem hiding this comment.
We add -> None to tests to trigger the linter. Actually not necessarily in this case as attribute has a type hint, but more consistent with other tests
Changes
Fix missing arguments to
WorkflowLinterLinked issues
Resolves #2808