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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Mar 17, 2021
Merged

Raise proper error message if duplicate macros found #3165

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5ed4af2
Raise proper error message if duplicate macros found
cgopalan Mar 14, 2021
31989b8
Fix flake8 errors
cgopalan Mar 14, 2021
5c50131
Fix failing test
cgopalan Mar 14, 2021
a8b2942
Fix duplicate macro path message
cgopalan Mar 16, 2021
0e958f3
Add to changelog
cgopalan Mar 16, 2021
f4c7496
Add to contributor list
cgopalan Mar 16, 2021
369b595
Merge branch 'develop' into dup-macro-message
jtcohen6 Mar 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion core/dbt/parser/results.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def add_macro(self, source_file: SourceFile, macro: ParsedMacro):
# subtract 2 for the "Compilation Error" indent
# note that the line wrap eats newlines, so if you want newlines,
# this is the result :(
msg = line_wrap_message(
dup_macro_msg = line_wrap_message(
f'''\
dbt found two macros named "{macro.name}" in the project
"{macro.package_name}".
Expand All @@ -140,6 +140,21 @@ def add_macro(self, source_file: SourceFile, macro: ParsedMacro):
''',
subtract=2
)
dup_path_msg = line_wrap_message(
f"""\
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
Copy link
Contributor

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:

Suggested change
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

Copy link
Contributor Author

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?

Copy link
Contributor

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.

  1. In the case where original_file_path != other_path, we should definitely return the dup_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.)

  2. 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 in macro-paths
    b. The same-named macro is defined more than once in the same file (nothing to do with macro-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.

Copy link
Contributor Author

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

""",
subtract=2,
)
msg = (
dup_path_msg
if macro.original_file_path == other_path
else dup_macro_msg
)
raise_compiler_error(msg)

self.macros[macro.unique_id] = macro
Expand Down