-
Notifications
You must be signed in to change notification settings - Fork 17
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
[Issue 2130] Update analytics documentation #3562
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Would love to merge in this PR to update/remove the stale content.
The only slightly urgent thing to fix is the note I left on an additional prerequisite for Postgres to be installed locally if running it natively.
The other comments are non-blocking, but would love to add the following items in future PRs:
- An updated overview of the file structure
- Guide for accessing the DB locally using psql
- Guide for working in Metabase -- should probably be its own file, mostly just links to existing documentation and Metabase data types, etc.
```text | ||
root | ||
├── analytics | ||
│ └── src | ||
│ └── analytics | ||
│ └── datasets Create re-usable data interfaces for calculating metrics | ||
│ └── integrations Integrate with external systems used to export data or metrics | ||
│ └── metrics Calculate the project's operational metrics | ||
│ └── tests | ||
│ └── integrations Integration tests, mostly for src/analytics/integrations | ||
│ └── datasets Unit tests for src/analytics/datasets | ||
│ └── metrics Unit tests for src/analytics/metrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this file structure has changed a little, but would it be possible to update it rather than simply removing it?
Having a high-level overview of the main sub-packages and key root level files in a codebase is often helpful to understand how to contribute to it.
|
||
![Screenshot of passing the --help flag to CLI entry point](../../analytics/static/screenshot-cli-help.png) | ||
|
||
Additional guidance on working with the CLI tool can be found in the [usage guide](usage.md#using-the-command-line-interface). | ||
## Example Development Tasks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't block merging this, but it would be good to add instructions for accessing the postgres DB using Docker and psql
or [asdf](https://asdf-vm.com/). | ||
- **Poetry:** After installing and activating the right version of Python, [install poetry with the official installer](https://python-poetry.org/docs/#installing-with-the-official-installer) or alternatively use [pipx to install](https://python-poetry.org/docs/#installing-with-pipx). | ||
- **Python version 3.12:** [pyenv](https://github.com/pyenv/pyenv#installation) is one popular option for installing Python, or [asdf](https://asdf-vm.com/) | ||
- **Poetry:** [install poetry with the official installer](https://python-poetry.org/docs/#installing-with-the-official-installer) or alternatively use [pipx to install](https://python-poetry.org/docs/#installing-with-pipx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up PR: #3598 |
## Summary Fixes #2130 ### Time to review: __2 mins__ ## Changes proposed > What was added, updated, or removed in this PR. A few minor additions to the analytics documentation, as requested in the comments on PR #3562 ## Context for reviewers > Testing instructions, background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers. Explain how the changes were verified. ## Additional information > Screenshots, GIF demos, code examples or output to help show the changes working as expected.
Summary
Fixes #2130
Time to review: 5 mins
Changes proposed
The analytics readme and other documentation was stale. The PR refreshes the content.
Context for reviewers
Additional information