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 Error Messaging for Invalid Source Arguments #1809

Merged
merged 4 commits into from
Oct 8, 2019
Merged

Improve Error Messaging for Invalid Source Arguments #1809

merged 4 commits into from
Oct 8, 2019

Conversation

elikastelein
Copy link
Contributor

This pull request fixes #1660

This pull request enforces that exactly two arguments are provided to source() during compilation. If anything other than 2 arguments are given, a compiler error is now raised. This is an improvement over the current state described in #1660, where the error messaging is unclear after providing a single argument to source().

#1660 also mentions adding better error messaging in ref() but it already seems satisfactory to me: https://github.com/fishtown-analytics/dbt/blob/dev/louisa-may-alcott/core/dbt/context/parser.py#L98-L107

This is my first contribution to dbt so I will need help with the CI environment variables.

Thanks to @emilieschario for helping me with this.

@cla-bot
Copy link

cla-bot bot commented Oct 5, 2019

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Eli Kastelein.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@drewbanin
Copy link
Contributor

awesome, this looks great @CaptainEli!!

Are you able to sign the attached CLA? If so, we can definitely merge this for 0.15.0 :D

@drewbanin
Copy link
Contributor

This looks pretty good to me -- going to kick off the CI suite now, but I think we'll me ready to merge this when the tests pass :)

Screen Shot 2019-10-07 at 5 41 55 PM

@cla-bot check

@cla-bot cla-bot bot added the cla:yes label Oct 7, 2019
@cla-bot
Copy link

cla-bot bot commented Oct 7, 2019

The cla-bot has been summoned, and re-checked this pull request!

@emilieschario
Copy link
Contributor

Great work @CaptainEli!!

@drewbanin
Copy link
Contributor

ack - there's just one pep8 issue here:

core/dbt/context/parser.py:118:80: E501 line too long (84 > 79 characters)

If you're able to make that line <= 79 characters, then we should be ready to go here :)

@elikastelein
Copy link
Contributor Author

@drewbanin thanks for the help. I've shortened it now 👍

Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CaptainEli (a great username, to be sure)! This is great, merging it for the 0.15.0 release :)

@drewbanin drewbanin merged commit 54d0230 into dbt-labs:dev/louisa-may-alcott Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better error messaging for malformed source/ref inputs
3 participants