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

Docs cleanup #620

Merged
merged 8 commits into from
Jul 18, 2022
Merged

Docs cleanup #620

merged 8 commits into from
Jul 18, 2022

Conversation

dbeatty10
Copy link
Contributor

This is a:

  • documentation update
  • bug fix with no breaking changes
  • new functionality
  • a breaking change

All pull requests from community contributors should target the main branch (default).

Description & motivation

The docs were out-of-date in a few areas:

  • GitHub URLs referring to fishtown-analytics org
  • name of default branch
  • other little bits

So this was a quick pass at cleaning up some areas. It was not fully comprehensive; I left a TODO regarding updating RELEASE.md.

Checklist

None of the following items were applicable in this case:

  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have added an entry to CHANGELOG.md (if applicable)

@dbeatty10 dbeatty10 requested a review from joellabes July 15, 2022 22:43
Copy link
Contributor

@joellabes joellabes left a comment

Choose a reason for hiding this comment

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

Wonderful improvements! Thank you 🙏

The only 🚩 is that the changelog's URLs seem to have been incorrectly find-and-replaced

CHANGELOG.md Outdated
@@ -214,7 +214,7 @@ After:

## Under the hood

- Declare compatibility with dbt v0.21.0, which has no breaking changes for this package ([#398](https://github.com/fishtown-analytics/dbt-utils/pull/398))
- Declare compatibility with dbt v0.21.0, which has no breaking changes for this package ([#398](https://github.com/https://github.com/dbt-labs/dbt-utils-analytics/dbt-utils/pull/398))
Copy link
Contributor

Choose a reason for hiding this comment

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

This file's URLs look pretty busted (two github.com references, dbt-utils-analytics) - did a find and replace go awry?

Beside that, I'm not averse to old changelog entries using the old format - I wouldn't have brought it up if the switch had been clean, but since it's not I wanted to point out that reverting the file was also a valid option - these links were accurate at the time they were built, and are basically read-only, and will continue to resolve in the right place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for noticing that find-and-replace gone awry! 👀 💪

Will make this update and re-request your review.

Agreed that the old URLs should continue to resolve just fine. I'd still prefer to update all references of "github.com/fishtown-analytics" to "github.com/dbt-labs/dbt-utils" so that it's clear during any grep operation that the repo is up-to-date and fully reflects the org name change.

Fix broken links
@dbeatty10 dbeatty10 requested a review from joellabes July 18, 2022 17:31
Copy link
Contributor

@joellabes joellabes left a comment

Choose a reason for hiding this comment

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

GOOD

@dbeatty10 dbeatty10 merged commit fbbc0fb into main Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants