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

improve get missing filepaths utils logic #105

Merged
merged 9 commits into from
Mar 14, 2023

Conversation

BAntonellini
Copy link
Collaborator

Replaced simple string work of get_missing_file_paths utils function for a proper pathlib.Path approach.

With these:

  • Only existent files are added to hooks
  • dbt project must no longer be nested in a subfolder: it can be at the same level of pre-commit-config.yml, or in any level of depth

@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (15cff54) 100.00% compared to head (3a31b0b) 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##              main      #105    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           45        47     +2     
  Lines         2041      2584   +543     
  Branches       252       272    +20     
==========================================
+ Hits          2041      2584   +543     
Impacted Files Coverage Δ
pre_commit_dbt/check_script_has_no_table_name.py 100.00% <100.00%> (ø)
pre_commit_dbt/check_source_has_freshness.py 100.00% <100.00%> (ø)
pre_commit_dbt/check_source_has_tests_by_group.py 100.00% <100.00%> (ø)
pre_commit_dbt/utils.py 100.00% <100.00%> (ø)

... and 42 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@katieclaiborne
Copy link
Contributor

@BAntonellini, is this improvement intended to resolve #101?

I found that pinning our pre-commit hooks to c3c3f1d resolved the issue, but didn't know if that was what you had in mind.

I did notice a seemingly transient IsADirectoryError in check-model-columns-have-desc, but wasn't sure if that was related to your changes.

Check the model columns have description.................................Failed
- hook id: check-model-columns-have-desc
- exit code: 1

Traceback (most recent call last):
  File "/home/runner/.cache/pre-commit/repoo9i55a7s/py_env-python3/bin/check-model-columns-have-desc", line 8, in <module>
    sys.exit(main())
  File "/home/runner/.cache/pre-commit/repoo9i55a7s/py_env-python3/lib/python3.10/site-packages/pre_commit_dbt/check_model_columns_have_desc.py", line 93, in main
    status_code, _ = check_column_desc(paths=args.filenames, manifest=manifest)
  File "/home/runner/.cache/pre-commit/repoo9i55a7s/py_env-python3/lib/python3.10/site-packages/pre_commit_dbt/check_model_columns_have_desc.py", line [41](https://github.com/cityblock/data-platform/actions/runs/4408977134/jobs/7724690930#step:11:42), in check_column_desc
    for item in itertools.chain(models, schemas):
  File "/home/runner/.cache/pre-commit/repoo9i55a7s/py_env-python3/lib/python3.10/site-packages/pre_commit_dbt/utils.py", line 201, in get_model_schemas
    with open(yml_file, "r") as file:
IsADirectoryError: [Errno 21] Is a directory: 'dbt/target/run/data_platform/models/staging/example/_example__models.yml'

@BAntonellini
Copy link
Collaborator Author

@katieclaiborne thanks for pointing that issue out, this PR is originally intended to solve what was proposed on PR #99

I've just committed a fix that should solve what you mentioned, as it's related with all these changes. Could you please try with 3a31b0b ?

Thank you

@katieclaiborne
Copy link
Contributor

That worked! Thank you, Bruno.

Did I also see you mention that v1.1 would be released this week?

@BAntonellini BAntonellini force-pushed the fix/improve-get-missing-filepaths branch from 90e2462 to 3a31b0b Compare March 14, 2023 13:57
@BAntonellini
Copy link
Collaborator Author

Great to hear that!

Yes, we are working some final details, and will release 1.1

@BAntonellini BAntonellini merged commit 806dd18 into main Mar 14, 2023
@BAntonellini BAntonellini deleted the fix/improve-get-missing-filepaths branch March 14, 2023 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants