Skip to content

Conversation

@JCZuurmond
Copy link
Contributor

Changes

Fix missing arguments to WorkflowLinter

Linked issues

Resolves #2808

@JCZuurmond JCZuurmond added integration test failure internal this pull request won't appear in release notes labels Oct 3, 2024
@JCZuurmond JCZuurmond self-assigned this Oct 3, 2024
@JCZuurmond JCZuurmond requested a review from a team as a code owner October 3, 2024 08:27
self.path_lookup,
TableMigrationIndex([]), # TODO: bring back self.tables_migrator.index()
self.directfs_access_crawler_for_paths,
self.used_tables_crawler_for_paths,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ericvergnaud : Is the right UsedTablescrawler?

@JCZuurmond
Copy link
Contributor Author

Failing integration tests will be covered in other PRs, see integration test failure issues.

@JCZuurmond
Copy link
Contributor Author

Want to reuse tests from #2788 to increase the test coverage which blocks the CI

@JCZuurmond JCZuurmond force-pushed the fix/workflow-linter-on-context branch from bb63a48 to b3590b5 Compare October 3, 2024 12:10
@github-actions
Copy link

github-actions bot commented Oct 3, 2024

✅ 42/42 passed, 1 flaky, 42m53s total

Flaky tests:

  • 🤪 test_fix_dashboard (1m26.769s)

Running from acceptance #6352

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:
Copy link
Contributor

Choose a reason for hiding this comment

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

That's bad. For clarity class factory methods should always come before instance methods

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 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"""
Copy link
Contributor

Choose a reason for hiding this comment

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

should not be None ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@nfx nfx merged commit e801889 into main Oct 3, 2024
6 checks passed
@nfx nfx deleted the fix/workflow-linter-on-context branch October 3, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal this pull request won't appear in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure: test_linter_from_context

4 participants