-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Raise proper error message if duplicate macros found #3165
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: @cgopalan |
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: @cgopalan |
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.
Thanks for picking this up @cgopalan! One comment from me about the language in the error message. Otherwise, this looks good to go.
Could you add a Changelog entry under v0.20.0 ("Fixes" or "Under the hood"), and add yourself to the list of Contributors?
core/dbt/parser/results.py
Outdated
The file {macro.original_file_path} for macro {macro.name} | ||
was parsed multiple times by dbt. This error happens when a | ||
path is duplicated in a dbt_project.yml configuration. | ||
Check your `dbt_project.yml` file path configurations and | ||
remove any duplicated paths to fix this error |
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.
The conditional check (macro.original_file_path == other_path
) will also return True
if a macro is defined multiple time, with the same name, in one file. Rather than try to disambiguate between these errors, I think it's fine to mention both possibilities in the error message:
The file {macro.original_file_path} for macro {macro.name} | |
was parsed multiple times by dbt. This error happens when a | |
path is duplicated in a dbt_project.yml configuration. | |
Check your `dbt_project.yml` file path configurations and | |
remove any duplicated paths to fix this error | |
The macro {macro.name} in file {macro.original_file_path} | |
was parsed multiple times by dbt. This error happens when a | |
macro is defined multiple times in the same file, or when a | |
path is duplicated in a dbt_project.yml configuration. | |
To fix this error, check: | |
- {macro.original_file_path} | |
- The `macro-paths` configurations in dbt_project.yml |
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.
@jtcohen6 thanks for the quick feedback. I was considering the case where the macro was in two file paths, for eg in macros
directory and also in say macros/some/other/dir
. In that case macro.original_file_path == other_path
should not return True
. Do we not want to address that case?
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.
Yes, I completely agree with your rationale.
-
In the case where
original_file_path != other_path
, we should definitely return thedup_macro_msg
error message that tells the user the path of each file in which the same-named macro is defined. (I believe we already do this.) -
In the case where
original_file_path == other_path
, that means either:
a. The same file is parsed more than once, on account of a mistake inmacro-paths
b. The same-named macro is defined more than once in the same file (nothing to do withmacro-paths
)
The preexisting error message handles only case 1. The current PR adds better error handling for case 2a; I believe the error message should mention the possibility of 2b as well.
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.
Ah, I get what you are saying now. Thanks, makes sense! I have updated with changes and also added to changelog
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.
Thanks for this @cgopalan!
resolves #2449
Description
Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.