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

Implemented individual overview functionality. #129

Merged
merged 11 commits into from
Aug 27, 2020

Conversation

Mr-Nobody99
Copy link
Contributor

The primary changes are to the routing (added a new route for project_overviews),
and the model tree (added a condition to the click event to route to project overview),
I also added an example docs block in the ci-project/models/README.md

I made the changes in such a way that if there are no project level overview block defined default functionality should be unaffected. (ie. display overview block as normal.)

Fixes #127

@drewbanin
Copy link
Contributor

this is really neat @Mr-Nobody99!! I think we'll need to re-compile the artifacts in the data folder to get this working in the PR build environment.

I largely feel pretty good about an approach like this, but I'm tagging @jtcohen6 to see if he has any additional thoughts. I imagine a good scheme being __{{ project_name }}__ instead of {{ project_name }}_overview, but that feels like a pretty minor detail to hammer out.

Nice work!

@Mr-Nobody99
Copy link
Contributor Author

Mr-Nobody99 commented Aug 24, 2020

Thank you much @drewbanin, glad to hear that, I look forward to any input @jtcohen6 has as well. I also went ahead and changed the naming format to __{{ project_name }}__ as you suggested.

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

This is a great idea! Thanks for the thoughtful work @Mr-Nobody99, I left one comment about overviews for installed packages.

if (overview.package_name != 'dbt') {
selected_overview = overview;
}
});

if (project_name !== null) {
let project_key = `${project_name}.__${project_name}__`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll want to let top-level projects "override" the project-level overviews of installed packages. If my_project installs the snowplow package, I would want my_project.__snowplow__ to take precedence over snowplow.__snowplow__, especially if the latter doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot @jtcohen6 I'm glad you like the idea, I agree with your comment completely. I added a new commit that I think should handle that scenario. Let me know if you think it needs any other change.

@Mr-Nobody99 Mr-Nobody99 requested a review from jtcohen6 August 27, 2020 14:52
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

This looks great! I just formalized the contribution process for this repo, in line with the process in the main dbt repo. Could you:


You can assign a unique overview for each project by adding a docs block in an .md file of your project
and giving it a name with the following convention __<project_name>__.
{% enddocs %}
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Could you add an entry for {% docs __dbt_utils__ %} below?
  • As Drew mentioned, to really see your change in the PR preview, we'll need to run dbt docs generate in the ci-project to replace the artifacts currently in data/. If you're up for doing this as part of this PR, great! If not, we can do it separately after merging.

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 suggestion I added a block for dbt_utils. I also generated docs in the ci-project and replaced the data/ files with them. I also signed the CLA. Please let me know if I did anything incorrectly with the data/ artifacts.

@jtcohen6
Copy link
Contributor

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Aug 27, 2020

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: @Mr-Nobody99

@cla-bot
Copy link

cla-bot bot commented Aug 27, 2020

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

@cla-bot cla-bot bot added the cla:yes label Aug 27, 2020
@Mr-Nobody99 Mr-Nobody99 requested a review from jtcohen6 August 27, 2020 18:10
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

This is awesome! Thanks for signing the CLA, adding a changelog entry, and updating the artifacts to test this in PR preview :)

Comment on lines 19 to 28
<<<<<<< HEAD
and giving it a name with the following convention { \__project_name__ }
{% enddocs %}

{% docs __dbt_utils__ %}
## DBT_UTILS package overview
This [dbt package](https://docs.getdbt.com/docs/building-a-dbt-project/package-management) is a collection of tools to help with common tasks.
=======
and giving it a name with the following convention __<project_name>__.
>>>>>>> parent of b2999b6... Added overview block for dbt_utils, and generated new docs from ci_project(replaced existing in data/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny thing: looks like there are a few git vestiges from merging

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 my bad, next commit should fix.

@jtcohen6 jtcohen6 merged commit 294f68b into dbt-labs:master Aug 27, 2020
@Mr-Nobody99 Mr-Nobody99 deleted the feature/individual-overviews branch September 28, 2020 21:29
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.

Individual project overviews
3 participants