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

fix docstring duplication error issue #2054 #2080

Merged
merged 2 commits into from
Feb 11, 2020
Merged

fix docstring duplication error issue #2054 #2080

merged 2 commits into from
Feb 11, 2020

Conversation

bubbomb
Copy link
Contributor

@bubbomb bubbomb commented Jan 30, 2020

This commit fixes the issue #2054

@cla-bot cla-bot bot added the cla:yes label Jan 30, 2020
@drewbanin
Copy link
Contributor

hey @bubbomb - can you rebase this onto dev/barbara-gittings? We're cutting an RC of 0.15.2 tomorrow and are no longer accepting PRs to the dev/0.15.2 branch.

This looks really great so far! I saw your issue comment about adding tests. What do you think about adding a new test case for a sample project with two identically named docs block?

You can add a new test class like this one:
https://github.com/fishtown-analytics/dbt/blob/e080bfc79ab11a4d145de4c8da9d955c5e136d92/test/integration/035_docs_blocks/test_docs_blocks.py#L130-L147

And a new directory like this one (eg. duplicated_names/) that looks something like this:
https://github.com/fishtown-analytics/dbt/tree/dev/0.15.2/test/integration/035_docs_blocks/invalid_name_models

I can kick off the test suite once some tests are added here :)

@bubbomb
Copy link
Contributor Author

bubbomb commented Jan 31, 2020

thanks @drewbanin this is exactly what I was looking for. I messed around with a few other tests for a while but couldn't get things to work quite the way I wanted to. I'll give this a shot and let you know.

@bubbomb
Copy link
Contributor Author

bubbomb commented Jan 31, 2020

Would you like me to close this PR and submit a new PR to the dev/barbara-gittings branch?

@drewbanin
Copy link
Contributor

It's up to you! You can totally do that, or if you're comfortable enough with git, you can also rebase your commits onto the latest dev/barbara-gittings and we can update the base branch in this PR!

I think that might look something like

git remote add fishtown https://github.com/fishtown-analytics/dbt.git
git fetch fishtown
git rebase -i fishtown/dev/barbara-gittings

If that becomes a big mess, you can certainly feel free to open a new PR for a branch off of dev/barbara-gittings :)

@bubbomb bubbomb changed the base branch from dev/0.15.2 to dev/barbara-gittings February 3, 2020 16:22
@drewbanin
Copy link
Contributor

It looks like there are still some stray commits in here. If you'd prefer, please feel free to close this one and open up another PR for your change :)

@bubbomb
Copy link
Contributor Author

bubbomb commented Feb 6, 2020

Yea, that was a little odd. I just saw that too. No worries though I updated it. Will this work or do you want me to open a new PR?

@beckjake
Copy link
Contributor

beckjake commented Feb 7, 2020

Hey @bubbomb, this PR looks great!

The new test is failing, I believe you need the commented out with self.assertRaises(...) - this code fails during compile time, not run time, so you have to catch an exception instead of expect_pass=False: Just one of those fun quirks of our integration test system.

@bubbomb
Copy link
Contributor Author

bubbomb commented Feb 7, 2020

@beckjake crud... I must have left that commented out after messing around with a few things. I'll get a fix up for it.

@drewbanin
Copy link
Contributor

@bubbomb can you mark this PR as "ready for review" if it's ready to merge? Thanks!

@bubbomb bubbomb marked this pull request as ready for review February 10, 2020 21:52
@beckjake beckjake merged commit 4e58589 into dbt-labs:dev/barbara-gittings Feb 11, 2020
@beckjake
Copy link
Contributor

Thanks so much for your contribution @bubbomb! This will get in to 0.16.0 (but missed the cutoff for beta1).

@bubbomb
Copy link
Contributor Author

bubbomb commented Feb 11, 2020

@beckjake @drewbanin no problem you guys were great to work with!

@beckjake beckjake mentioned this pull request Mar 4, 2020
4 tasks
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.

3 participants