-
Notifications
You must be signed in to change notification settings - Fork 1
feat: updated insight names #78
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
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.
Important
Looks good to me! 👍
Reviewed everything up to 90c904b in 1 minute and 44 seconds. Click for details.
- Reviewed
702lines of code in54files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/core/platforms/dbt/insights/structure/test_directory_structure.py:56
- Draft comment:
The generate() loop iterates over each test's dependencies individually and creates an insight for every dependency with a directory mismatch. Consider aggregating mismatches per test so that if multiple dependencies have differing expected directories, a single aggregated insight is provided. This would improve clarity and reduce redundant alerts. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/datapilot/core/platforms/dbt/insights/checks/check_column_desc_are_same.py:14
- Draft comment:
Typographical note: The DESCRIPTION string reads "Column description for the same column name should be same ". Consider revising it to "Column description for the same column name should be the same" (removing the trailing space as well). - Reason this comment was not posted:
Comment was on unchanged code.
3. src/datapilot/core/platforms/dbt/insights/checks/check_source_columns_have_desc.py:19
- Draft comment:
There's a trailing space at the end of the string inREASON_TO_FLAG. Please remove the extra space. - Reason this comment was not posted:
Comment was on unchanged code.
4. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_all_columns.py:20
- Draft comment:
There appears to be an extra trailing space at the end of the string in the REASON_TO_FLAG assignment. Consider removing it for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
5. src/datapilot/core/platforms/dbt/insights/dbt_test/test_coverage.py:19
- Draft comment:
There appears to be an extra trailing space in the DESCRIPTION string on line 19. Consider removing it if it's unintentional. - Reason this comment was not posted:
Comment was on unchanged code.
6. src/datapilot/core/platforms/dbt/insights/modelling/downstream_models_dependent_on_source.py:22
- Draft comment:
The updated NAME value uses "downstream_source_dependency" while ALIAS is still "downstream_source_dependence". This inconsistency in the suffix (dependency vs dependence) might be unintentional. Please verify if they should match for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is asking for verification rather than stating a clear issue. "Dependence" and "dependency" are essentially the same word with slightly different forms. NAME appears to be used for internal identification while ALIAS is used for configuration purposes (seen in get_severity call). There's no technical requirement that they match exactly. The inconsistency could potentially cause confusion for developers looking at the code. Maybe there's a style guide that requires consistency between NAME and ALIAS. Without seeing a style guide that requires this consistency, this seems like a minor stylistic nitpick. The comment also asks for verification rather than making a clear recommendation. Delete the comment. It asks for verification rather than stating a clear issue, and the inconsistency between dependency/dependence is too minor to warrant a comment.
7. src/datapilot/core/platforms/dbt/insights/performance/chain_view_linking.py:19
- Draft comment:
Typographical issue: The description string contains trailing whitespace after 'slow computation'. Consider removing the extra space. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Q87bkC0UWde4fDa6
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Caution
Changes requested ❌
Reviewed 9b18329 in 3 minutes and 4 seconds. Click for details.
- Reviewed
708lines of code in54files - Skipped
0files when reviewing. - Skipped posting
64draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/core/platforms/dbt/insights/modelling/downstream_models_dependent_on_source.py:12
- Draft comment:
Typo in insight NAME: 'model_downstrsam_source_dependency' appears to have a misspelling. Consider changing it to 'model_downstream_source_dependency' for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/datapilot/core/platforms/dbt/insights/modelling/multiple_sources_joined.py:16
- Draft comment:
Insight name updated to 'multi_source_joins'. The new naming is concise and follows snake_case convention. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/datapilot/core/platforms/dbt/insights/modelling/model_fanout.py:15
- Draft comment:
Insight name updated to 'model_excessive_fanout'. This naming clearly communicates the issue; looks good. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/datapilot/core/platforms/dbt/insights/modelling/hard_coded_references.py:18
- Draft comment:
Insight name changed to 'hardcoded_refs'. The new name is succinct and consistent with naming conventions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. src/datapilot/core/platforms/dbt/insights/modelling/joining_of_upstream_concepts.py:16
- Draft comment:
Insight name updated to 'upstream_rejoins'. This concise name effectively reflects the check’s purpose. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/datapilot/core/platforms/dbt/insights/modelling/root_model.py:16
- Draft comment:
Insight name updated to 'orphan_models'; the new name clearly indicates models with no direct parents. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. src/datapilot/core/platforms/dbt/insights/modelling/source_fanout.py:15
- Draft comment:
Updated insight name to 'source_excessive_fanout'. The new identifier is consistent and descriptive. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. src/datapilot/core/platforms/dbt/insights/modelling/staging_model_dependent_on_downstream_models.py:16
- Draft comment:
Insight name updated to 'staging_downstream_dependency'. Verify that this change aligns with the dependency logic. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. src/datapilot/core/platforms/dbt/insights/modelling/staging_model_dependent_on_staging_models.py:16
- Draft comment:
Insight name updated to 'model_staging_on_staging'; naming is clear and concise. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. src/datapilot/core/platforms/dbt/insights/modelling/unused_sources.py:16
- Draft comment:
Insight name updated to 'unused_source'. This new identifier is succinct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
11. src/datapilot/core/platforms/dbt/insights/performance/chain_view_linking.py:15
- Draft comment:
Insight name updated to 'model_excessive_chain_of_views'. Threshold and description seem appropriate. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
12. src/datapilot/core/platforms/dbt/insights/performance/exposure_parent_materializations.py:17
- Draft comment:
Insight name updated to 'exposure_direct_source_dependency'. The new name clearly reflects the check's intent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
13. src/datapilot/core/platforms/dbt/insights/sql/sql_check.py:33
- Draft comment:
Insight name updated to 'model_unoptimized_sql'; the naming aligns with our internal standards. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
14. src/datapilot/core/platforms/dbt/insights/structure/model_directories_structure.py:20
- Draft comment:
Updated insight name to 'model_invalid_directory_structure'. This new name clearly reflects the issue. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
15. src/datapilot/core/platforms/dbt/insights/structure/model_naming_conventions.py:20
- Draft comment:
Insight name updated to 'model_invalid_name'. The update strictly enforces naming conventions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
16. src/datapilot/core/platforms/dbt/insights/structure/source_directories_structure.py:17
- Draft comment:
Insight name updated to 'source_invalid_directory_structure'. The new name is clear and concise. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
17. src/datapilot/core/platforms/dbt/insights/structure/test_directory_structure.py:16
- Draft comment:
Insight name updated to 'test_invalid_directory'. This name succinctly describes the issue. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
18. src/datapilot/core/platforms/dbt/insights/checks/check_source_columns_have_desc.py:15
- Draft comment:
Insight name updated to 'source_column_no_description'. The new identifier is clear and self‐explanatory. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
19. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_all_columns.py:17
- Draft comment:
Insight name updated to 'source_missing_columns'. Ensure that metadata and reporting reflect this new key. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
20. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_freshness.py:11
- Draft comment:
Insight name updated to 'source_no_freshness'. The updated name is concise; verify that configuration defaults are updated accordingly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
21. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_loader.py:11
- Draft comment:
Insight name updated to 'source_no_loader'. The new name is clear and aligns with our conventions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
22. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_meta_keys.py:15
- Draft comment:
Insight name updated to 'source_invalid_meta_keys'. This new identifier is clear; ensure config schema and defaults are correct. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
23. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_tests.py:10
- Draft comment:
Insight name updated to 'source_no_tests'. The update is straightforward. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
24. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_tests_by_group.py:11
- Draft comment:
Insight name updated to 'source_insufficient_tests_by_group'. The change is clear; verify that configuration schema matches changes. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
25. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_tests_by_name.py:11
- Draft comment:
Insight name updated to 'source_missing_required_tests'. The new name is more descriptive of the violation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
26. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_tests_by_type.py:11
- Draft comment:
Insight name updated to 'source_insufficient_tests_by_type'. Ensure that all key names in the config schema are updated accordingly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
27. src/datapilot/core/platforms/dbt/insights/governance/documentation_on_stale_columns.py:17
- Draft comment:
Insight name updated to 'model_stale_column_desc'. The new identifier is concise and clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
28. src/datapilot/core/platforms/dbt/insights/governance/exposures_dependent_on_private_models.py:17
- Draft comment:
Insight name updated to 'exposure_private_dependency'. This new name clearly indicates the check's purpose. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
29. src/datapilot/core/platforms/dbt/insights/governance/public_models_without_contracts.py:16
- Draft comment:
Insight name updated to 'public_model_no_contract'. The update streamlines the identifier. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
30. src/datapilot/core/platforms/dbt/insights/governance/undocumented_columns.py:19
- Draft comment:
Insight name updated to 'model_undocumented_columns'. The new name is precise; verify that the metadata is updated accordingly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
31. src/datapilot/core/platforms/dbt/insights/governance/undocumented_public_models.py:15
- Draft comment:
Insight name updated to 'undocumented_public_models'. The new naming aligns with our internal standards. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
32. src/datapilot/core/platforms/dbt/insights/modelling/direct_join_to_source.py:24
- Draft comment:
Insight name updated to 'direct_join_to_source'. The new identifier is clear and descriptive. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
33. src/datapilot/core/platforms/dbt/insights/modelling/duplicate_sources.py:18
- Draft comment:
Insight name updated to 'duplicate_source'. This concise name clearly indicates the issue. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
34. src/datapilot/core/platforms/dbt/insights/modelling/hard_coded_references.py:18
- Draft comment:
Insight name 'hardcoded_refs' is updated and consistent with our naming guidelines. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
35. src/datapilot/core/platforms/dbt/insights/modelling/joining_of_upstream_concepts.py:16
- Draft comment:
Insight name updated to 'upstream_rejoins'; the new name is concise and self-explanatory. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
36. src/datapilot/core/platforms/dbt/insights/modelling/model_fanout.py:16
- Draft comment:
Insight name updated to 'model_excessive_fanout'; this new name is clear and indicates potential high complexity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
37. src/datapilot/core/platforms/dbt/insights/modelling/multiple_sources_joined.py:16
- Draft comment:
Insight name updated to 'multi_source_joins'; naming is consistent and descriptive. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
38. src/datapilot/core/platforms/dbt/insights/modelling/root_model.py:16
- Draft comment:
Insight name updated to 'orphan_models'; this new name effectively indicates models with no direct parents. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
39. src/datapilot/core/platforms/dbt/insights/modelling/source_fanout.py:15
- Draft comment:
Insight name updated to 'source_excessive_fanout' which is clear and consistent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
40. src/datapilot/core/platforms/dbt/insights/modelling/staging_model_dependent_on_downstream_models.py:16
- Draft comment:
Insight name 'staging_downstream_dependency' updated; ensure that downstream configuration remains intact. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
41. src/datapilot/core/platforms/dbt/insights/modelling/staging_model_dependent_on_staging_models.py:16
- Draft comment:
Insight name updated to 'model_staging_on_staging'; the new name is succinct and clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
42. src/datapilot/core/platforms/dbt/insights/modelling/unused_sources.py:16
- Draft comment:
Insight name updated to 'unused_source' for clarity. The update simplifies the identifier. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
43. src/datapilot/core/platforms/dbt/insights/performance/chain_view_linking.py:15
- Draft comment:
Insight name updated to 'model_excessive_chain_of_views'; this naming is descriptive and aligns with performance concerns. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
44. src/datapilot/core/platforms/dbt/insights/performance/exposure_parent_materializations.py:17
- Draft comment:
Insight name updated to 'exposure_direct_source_dependency'; the new name clearly communicates the issue. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
45. src/datapilot/core/platforms/dbt/insights/sql/sql_check.py:33
- Draft comment:
Insight name updated to 'model_unoptimized_sql'. Naming is consistent with optimization feedback. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
46. src/datapilot/core/platforms/dbt/insights/structure/model_directories_structure.py:20
- Draft comment:
Insight name updated to 'model_invalid_directory_structure'. This update clearly indicates the folder issue. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
47. src/datapilot/core/platforms/dbt/insights/structure/model_naming_conventions.py:20
- Draft comment:
Insight name updated to 'model_invalid_name'. The new naming enforces expected conventions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
48. src/datapilot/core/platforms/dbt/insights/structure/source_directories_structure.py:17
- Draft comment:
Insight name updated to 'source_invalid_directory_structure'. The update is clear and follows naming standards. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
49. src/datapilot/core/platforms/dbt/insights/structure/test_directory_structure.py:16
- Draft comment:
Insight name updated to 'test_invalid_directory'. The new name is concise and descriptive. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
50. src/datapilot/core/platforms/dbt/insights/checks/check_macro_args_have_desc.py:11
- Draft comment:
Insight name updated to 'macro_arg_no_desc'. This update is clear and concise. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
51. src/datapilot/core/platforms/dbt/insights/checks/check_macro_has_desc.py:11
- Draft comment:
Insight name updated to 'macro_no_docs'. The change is straightforward and consistent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
52. src/datapilot/core/platforms/dbt/insights/checks/check_column_name_contract.py:16
- Draft comment:
Insight name updated to 'column_name_pattern_violation'. This makes the intent of the check explicit. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
53. src/datapilot/core/platforms/dbt/insights/checks/check_column_desc_are_same.py:14
- Draft comment:
Typo suggestion: The DESCRIPTION string contains a trailing space and the phrasing 'should be same' might be clearer if written as 'should be the same'. Consider removing the extra space and revising the text. - Reason this comment was not posted:
Comment was on unchanged code.
54. src/datapilot/core/platforms/dbt/insights/checks/check_model_parents_database.py:13
- Draft comment:
Typo/grammar suggestion: In the DESCRIPTION, consider adding an article for clarity. Instead of "Ensures the parent models or sources are from certain database.", it may be clearer as "Ensures the parent models or sources are from a certain database." - Reason this comment was not posted:
Comment was on unchanged code.
55. src/datapilot/core/platforms/dbt/insights/checks/check_source_childs.py:13
- Draft comment:
Typo: In the DESCRIPTION, the term "childs" is used. Consider replacing it with "children" for proper grammar. - Reason this comment was not posted:
Comment was on unchanged code.
56. src/datapilot/core/platforms/dbt/insights/checks/check_source_childs.py:14
- Draft comment:
Typo: In REASON_TO_FLAG, the word "childs" appears. It might be clearer to use "children" instead. - Reason this comment was not posted:
Comment was on unchanged code.
57. src/datapilot/core/platforms/dbt/insights/checks/check_source_childs.py:15
- Draft comment:
Typo: The string constant MIN_CHILDS_STR uses "childs". Consider renaming it to "min_children" for consistency and proper grammar. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
58. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_all_columns.py:20
- Draft comment:
There's an extra trailing space at the end of the REASON_TO_FLAG string. Consider removing it to ensure consistency. - Reason this comment was not posted:
Comment was on unchanged code.
59. src/datapilot/core/platforms/dbt/insights/governance/exposures_dependent_on_private_models.py:18
- Draft comment:
There appears to be an extra trailing space at the end of the description string (line 18). Consider removing it to avoid unintended formatting issues. - Reason this comment was not posted:
Comment was on unchanged code.
60. src/datapilot/core/platforms/dbt/insights/modelling/model_fanout.py:18
- Draft comment:
There's a trailing whitespace at the end of the DESCRIPTION string. It might be best to remove the space after 'children.' - Reason this comment was not posted:
Comment was on unchanged code.
61. src/datapilot/core/platforms/dbt/insights/performance/chain_view_linking.py:19
- Draft comment:
There appears to be an extraneous trailing space at the end of the DESCRIPTION string. This could lead to subtle issues and should be removed. - Reason this comment was not posted:
Comment was on unchanged code.
62. src/datapilot/core/platforms/dbt/insights/sql/sql_check.py:35
- Draft comment:
There is a trailing space at the end of the DESCRIPTION string. Consider removing it for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
63. src/datapilot/core/platforms/dbt/insights/sql/sql_check.py:37
- Draft comment:
There is a trailing space at the end of the FAILURE_MESSAGE string. Consider removing it for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
64. src/datapilot/core/platforms/dbt/insights/structure/source_directories_structure.py:19
- Draft comment:
There's a trailing whitespace at the end of the string on this line. Consider removing the extra space to maintain consistency. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Y4UXJIdwTzAKivQK
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
|
||
| class CheckSourceColumnsHaveDescriptions(ChecksInsight): | ||
| NAME = "Source columns have descriptions" | ||
| NAME = "source_column_no_description" |
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.
Typo: The NAME constant uses the singular form ('source_column_no_description') while the rest of the file references plural form (e.g., 'columns'). Consider using a plural form (e.g., 'source_columns_no_description') for consistency.
| NAME = "source_column_no_description" | |
| NAME = "source_columns_no_description" |
|
|
||
| NAME = "Downstream models dependent on source" | ||
| ALIAS = "downstream_source_dependence" | ||
| NAME = "model_downstrsam_source_dependency" |
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.
There appears to be a typographical error in the value of NAME. It currently reads "model_downstrsam_source_dependency" but likely should be "model_downstream_source_dependency".
| NAME = "model_downstrsam_source_dependency" | |
| NAME = "model_downstream_source_dependency" |
Update NAME attributes across all insight classes from verbose descriptions
to concise snake_case identifiers for better programmatic usage. This change
affects 54 files across checks, tests, governance, modelling, performance,
SQL, and structure insights.
Key changes:
- Check insights: Use descriptive snake_case (e.g., column_name_inconsistent_desc)
- Test insights: Standardize naming (e.g., missing_primary_keys_tests)
- Governance insights: Clarify purpose (e.g., public_model_no_contract)
- Modelling insights: Simplify names (e.g., direct_join_to_source)
- Performance insights: Concise identifiers (e.g., chain_view_linking)
- Structure insights: Pattern-based naming (e.g., model_path_violation)
Also fixes typo in check_model_name_contract.py ("Mmdel" -> proper identifier)
ALIAS and DESCRIPTION fields remain unchanged for backward compatibility
and human-readable documentation.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
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.
Important
Looks good to me! 👍
Reviewed d968896 in 2 minutes and 25 seconds. Click for details.
- Reviewed
709lines of code in54files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_tests_by_type.py:21
- Draft comment:
Undefined attribute 'TEST_NAME_STR' used in the dict comprehension. Likely should check using 'TEST_TYPE_STR' instead. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_tests_by_name.py:42
- Draft comment:
Missing required argument 'missing_tests' when calling _build_failure_result; ensure you pass the missing tests list. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_tests_by_name.py:68
- Draft comment:
The helper _source_has_tests_by_name returns a tuple (bool, missing_tests) but its usage seems inconsistent; consider reworking its API for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. src/datapilot/core/platforms/dbt/insights/checks/check_model_has_meta_keys.py:15
- Draft comment:
Typo detected on line 15: The NAME variable is set to "model_invalid_meta_keys", which conflicts with the ALIAS and DESCRIPTION that indicate the model should have valid meta keys. Did you mean to use "model_has_valid_meta_keys"? - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The current NAME "model_invalid_meta_keys" actually makes more sense than the suggested change. This check flags models when they have invalid/missing meta keys (see _build_failure_result). The NAME should reflect what's wrong, not what's desired. The ALIAS and DESCRIPTION describe the check itself, while NAME is used in error reporting. I could be wrong about the naming convention - maybe there's a consistent pattern across other checks that this violates. Also, the original author might have had a specific reason for wanting to emphasize the positive case. Even without knowing the broader context, the current negative NAME makes more sense for error reporting, which is its primary use. The positive ALIAS is fine for referring to the check itself. The current NAME accurately reflects what the check flags (invalid meta keys), while ALIAS describes what the check verifies. The comment should be deleted.
5. src/datapilot/core/platforms/dbt/insights/checks/check_source_columns_have_desc.py:19
- Draft comment:
Typo: There is extraneous trailing whitespace at the end of the REASON_TO_FLAG string. Please remove the extra space after 'analysis.' - Reason this comment was not posted:
Comment was on unchanged code.
6. src/datapilot/core/platforms/dbt/insights/checks/check_source_has_all_columns.py:20
- Draft comment:
Typographical note: There is a trailing whitespace after 'analysis.' which should be removed for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide a specific code suggestion or highlight a potential issue with the code logic. It focuses on a minor typographical issue that does not affect the functionality of the code.
7. src/datapilot/core/platforms/dbt/insights/dbt_test/missing_primary_key_tests.py:24
- Draft comment:
Typographical note: The NAME string "missing_primary_keys_tests" uses a plural "keys" while the ALIAS is "missing_primary_key_tests" (singular). Please verify if this discrepancy is intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% Looking at the code, this is a class constant naming issue. While it's true there's an inconsistency, there's no strong evidence that this is actually a problem. The NAME and ALIAS could intentionally be different. Neither format is clearly wrong, and this seems more like a style preference than a real issue. The comment is asking for verification rather than pointing out a clear problem. The inconsistency could potentially cause confusion in logs or UI where both values are displayed. It might be worth standardizing. While standardizing might be nice, there's no evidence this is causing any actual problems. The comment is asking for verification rather than pointing out a clear issue that needs fixing. Delete the comment. It's asking for verification ("Please verify if...") which violates our rules, and there's no strong evidence that the current naming is problematic.
8. src/datapilot/core/platforms/dbt/insights/modelling/model_fanout.py:18
- Draft comment:
Typo: There is a trailing space at the end of the DESCRIPTION string. Consider removing the extra space. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_EUtZbs9AujZ1Ouax
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
LGTM
Link: https://altimateai.atlassian.net/browse/AI-4111?focusedCommentId=16818
Important
Standardize
NAMEattribute for variousChecksInsightclasses across multiple files to ensure consistent naming conventions.NAMEinCheckColumnDescAreSame,CheckColumnNameContract,CheckMacroArgsHaveDesc,CheckMacroHasDesc,CheckModelHasAllColumns,CheckModelHasMetaKeys,CheckModelHasPropertiesFile,CheckModelHasTestsByGroup,CheckModelHasTestsByName,CheckModelHasTestsByType,CheckModelMaterializationByChilds,CheckModelNameContract,CheckModelParentsAndChilds,CheckModelParentsDatabase,CheckModelParentsSchema,CheckModelTags,CheckSourceChilds,CheckSourceColumnsHaveDescriptions,CheckSourceHasAllColumns,CheckSourceHasFreshness,CheckSourceHasLoader,CheckSourceHasMetaKeys,CheckSourceHasTests,CheckSourceHasTestsByGroup,CheckSourceHasTestsByName,CheckSourceHasTestsByType,CheckSourceTableHasDescription,CheckSourceTags.NAMEinMissingPrimaryKeyTests,DBTTestCoverage.NAMEinDBTDocumentationStaleColumns,DBTExposureDependentOnPrivateModels,DBTPublicModelWithoutContracts,DBTMissingDocumentation,DBTUndocumentedPublicModels.NAMEinDBTDirectJoinSource,DBTDownstreamModelsDependentOnSource,DBTDuplicateSources,DBTHardCodedReferences,DBTRejoiningOfUpstreamConcepts,DBTModelFanout,DBTModelsMultipleSourcesJoined,DBTRootModel,DBTSourceFanout,DBTStagingModelsDependentOnDownstreamModels,DBTStagingModelsDependentOnStagingModels,DBTUnusedSources.NAMEinDBTChainViewLinking,DBTExposureParentMaterialization.NAMEinSqlCheck.NAMEinDBTModelDirectoryStructure,DBTModelNamingConvention,DBTSourceDirectoryStructure,DBTTestDirectoryStructure.This description was created by
for d968896. You can customize this summary. It will automatically update as commits are pushed.