-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Read filenames using case insensitive extension #2263
Read filenames using case insensitive extension #2263
Conversation
Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA. In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin. CLA has not been signed by users: @sumanau7 |
@cla-bot check |
The cla-bot has been summoned, and re-checked this pull request! |
Thanks for your contribution @sumanau7! It looks like flake8 is unhappy with your changes, mostly line length things - once those are fixed, tests should work:
|
f78d822
to
6b13d45
Compare
# TODO: In order to support this, make FilesystemSearcher accept a list | ||
# of file patterns. eg: ['.yml', '.yaml'] |
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.
You should be able to just return itertools.chain(yaml_files, yml_files)
. It won't be the most efficient thing in the world, but that's probably fine.
@beckjake Does the overall PR looks good to be merged, for the new warning added, is the file |
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 think this unit test looks good!
I think for the warning, this should get an integration test. The tests in test/integration/008_schema_tests_test
are a sort of reasonable place to start. In particular, check out the TestQuotedSchemaTestColumns
test, which makes use of the quote-required-models
folder inn there. You'll want to do similar to that test:
- create a models folder for the test
- put a model ending with (
.sql
) in it. It can just be something likeselect 1 as id
. - put a schema.yml file with
.yaml
in there and declare a test on the model (probably just aunique
test onid
) - make sure
results = self.run_dbt(["run"], strict=False)
passes and thatlen(results) == 1
- make sure
results = self.run_dbt(["test"], strict=False)
passes and thatlen(results) == 1
- make sure
results = self.run_dbt(["run"])
fails with an error (because strict mode is enabled). You will probably have to use something like this:
with self.assertRaises(dbt.exceptions.Exception) as exc:
self.run_dbt(["run"])
self.assertIn('yaml', str(exc.exception)
You'll also want to add a second test that does the same but uses a file ending in .SQL
, I think. That test might have to get skipped on windows.
core/dbt/parser/schemas.py
Outdated
self.project, self.project.all_source_paths, '.yaml' | ||
) | ||
if yaml_files: | ||
logger.warning( |
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 think instead of logger.warning
, we want exceptions.warn_or_error
here.
@sumanau7 This is a great PR so far, thanks for your contribution! This PR definitely merits some tests (I don't want to regress and break this in the future!) and we have some internal conventions on how we handle warnings that I have suggestions on, but this is all very minor. |
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Sumanau Sareen.
|
c7acc8d
to
1b8d58b
Compare
edb3a72
to
647b3ba
Compare
@beckjake let me know if integration tests looks good, i will proceed to update changes in changelog |
647b3ba
to
527e6c2
Compare
…egration test for checking case-insensitive extensions
527e6c2
to
f9343c4
Compare
Ok, thanks! I've kicked off tests again and I truly believe this is it, though no promises 😆 I appreciate you sticking with this, I know it's a pain! |
Don't worry about it, you guys are the best. |
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Sumanau Sareen.
|
70628d1
to
6f302e2
Compare
This will make it into 0.17.0 when that's released. Thanks so much for contributing to dbt! |
…ng_case_insenstive_extension Read filenames using case insensitive extension
resolves #1681
Description
Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.