examples/: add a README explaining what's there#6
examples/: add a README explaining what's there#6SwathiMystery merged 3 commits intotownsquare-os:mainfrom
Conversation
SwathiMystery
left a comment
There was a problem hiding this comment.
Thanks for picking this up @badoni003shreyansh — really clean first PR. Most of issue #3 is delivered: 2-line intro ✓, scripts table with empty-placeholder ✓, "how to run" section that points at SELF_HOSTING.md ✓.
A few requested changes before merge — all small. No blockers, just tightening so the doc doesn't drift from reality.
Required changes
1. Drop or soften the "import townsquare modules directly — no HTTP calls, no Docker exec" line.
Issue #3's optional follow-up (examples/01_quick_query.py) explicitly suggested "uses the running server's /ask endpoint (with a session cookie or a simple admin bypass) to demonstrate the federated answer flow programmatically" — i.e., HTTP calls are expected, at least sometimes. Both shapes are valid examples. Suggested rewrite:
Runnable scripts that show one feature each. Some import
townsquare.*directly (library usage); others hit a running server's HTTP endpoints (integration patterns).
2. The pip install -e ".[google,all]" instruction is misleading.
google is already a subset of all in pyproject.toml — listing both is redundant. Also, examples don't necessarily need every extra. Suggested:
pip install -e ".[dev]" # core + dev tooling, enough for most examples
pip install -e ".[all,dev]" # also installs slack-sdk, notion-client, pygithubOr simply pip install -e . if the example only uses what's already in dependencies.
3. Cross-reference the issue properly in the PR body.
The current PR body links to a generic GitHub issue-search URL (/issues/assigned?issue=townsquare-os%7C…) that no one can resolve. Standard pattern is Fixes #3 so GitHub auto-closes the issue on merge. Drop the existing line and add:
Fixes #3
Nits (optional but appreciated)
4. The HTML comment <!-- When adding: script name, one-line description. Update this table. --> is helpful as guidance but it's redundant with the explicit instruction at the bottom ("When you add a script, update the table above."). Pick one — I'd keep the one at the bottom since it's plain text and visible in any markdown viewer.
5. The empty-table placeholder row reads slightly awkwardly. A slightly cleaner version:
| Script | What it does |
| --- | --- |
| _(none yet — be the first contributor!)_ | |(or just leave the table empty with the column headers — readers understand.)
6. "Each script reads .env from the repo root" is a great rule to set, but it's not enforced anywhere. Consider adding to the "What makes a good example" section: "Loads .env from the repo root via python-dotenv or townsquare.settings.get_settings()." — gives the next contributor a concrete pattern to copy.
What's good
- Tone is right — conversational without being chatty.
- "What makes a good example" section is a nice unprompted addition; it'll save reviewers time on follow-up PRs.
- Smart to include the
pip install -estep explicitly. New contributors trip over editable installs constantly. - Clean markdown formatting.
Once items 1–3 are addressed, this is good to merge. Items 4–6 are at your discretion.
Welcome to the project!
|
Hey @SwathiMystery Thanks for the review. I have considered the changes and updated the doc accordingly. Pls review . |
SwathiMystery
left a comment
There was a problem hiding this comment.
All requested changes addressed:
- ✓ #1 — opening line now correctly mentions both library usage AND HTTP endpoints. The framing reads exactly right.
- ✓ #2 —
pip installblock now shows three correct options (.[dev],.[all,dev], plain.) with a one-line guide for when to use each. Removed the misleading.[google,all]. - ✓ #3 — PR body now
Fixes #3— the issue will auto-close on merge. - ✓ #4 — HTML comment dropped, single instruction at the bottom kept.
- ✓ #5 — placeholder row reads cleanly now:
_(none yet — be the first contributor!)_. - ✓ #6 — "Loads
.envfrom the repo root (e.g., viapython-dotenvortownsquare.settings.get_settings())" added as a bullet under "What makes a good example". Future contributors have a concrete pattern to copy.
Quick note on the merge commit (652aac3): you merged main into the branch instead of rebasing, which adds a merge commit to the history. The repo's branch protection has required_linear_history: true, so the merge button on this PR will produce a squash anyway — no action needed from you, just FYI for next time. For PR-style work in this repo, prefer git rebase main over git merge main to keep branch history flat.
Thank you for the careful follow-up. Welcome to the project, and looking forward to seeing your first example script (issue #3 mentioned examples/01_quick_query.py as a possible follow-up if you want to keep going). LGTM, merging.
Fixes #3