164 lines
4.2 KiB
Markdown
164 lines
4.2 KiB
Markdown
|
# Reviewing Geolog
|
||
|
|
|
||
|
|
This guide is for humans reviewing the codebase, especially when you want to answer:
|
||
|
|
|
||
|
|
- where a behavior lives,
|
||
|
|
- which commands to run first,
|
||
|
|
- and which modules are most likely to matter for a given bug or change.
|
||
|
|
|
||
|
|
## Quick Start
|
||
|
|
|
||
|
|
If you have `just` installed:
|
||
|
|
|
||
|
|
```bash
|
||
|
|
just review
|
||
|
|
just review-gui
|
||
|
|
```
|
||
|
|
|
||
|
|
Without `just`:
|
||
|
|
|
||
|
|
```bash
|
||
|
|
cargo check
|
||
|
|
cargo test
|
||
|
|
cargo check --features gui --bin geolog-gui
|
||
|
|
cargo test --features gui
|
||
|
|
```
|
||
|
|
|
||
|
|
## Best Reading Order
|
||
|
|
|
||
|
|
For most reviews, this order gives the fastest mental model:
|
||
|
|
|
||
|
|
1. `README.md`
|
||
|
|
- User-facing behavior and examples.
|
||
|
|
2. `docs/ARCHITECTURE.md`
|
||
|
|
- High-level subsystem map.
|
||
|
|
3. `src/lib.rs`
|
||
|
|
- Top-level module layout.
|
||
|
|
4. Syntax pipeline
|
||
|
|
- `src/ast.rs`
|
||
|
|
- `src/lexer.rs`
|
||
|
|
- `src/parser.rs`
|
||
|
|
- `src/pretty.rs`
|
||
|
|
5. Semantic core
|
||
|
|
- `src/core.rs`
|
||
|
|
- `src/id.rs`
|
||
|
|
- `src/universe.rs`
|
||
|
|
6. Elaboration
|
||
|
|
- `src/elaborate/mod.rs`
|
||
|
|
- `src/elaborate/theory.rs`
|
||
|
|
- `src/elaborate/instance.rs`
|
||
|
|
7. Runtime and persistence
|
||
|
|
- `src/repl.rs`
|
||
|
|
- `src/store/mod.rs`
|
||
|
|
- `src/store/theory.rs`
|
||
|
|
- `src/store/instance.rs`
|
||
|
|
8. Query and chase
|
||
|
|
- `src/query/mod.rs`
|
||
|
|
- `src/query/chase.rs`
|
||
|
|
- `src/query/compile.rs`
|
||
|
|
- `src/query/backend.rs`
|
||
|
|
9. Optional layers
|
||
|
|
- `src/solver/`
|
||
|
|
- `src/gui/`
|
||
|
|
|
||
|
|
## How To Map A Change
|
||
|
|
|
||
|
|
Use this shortcut table:
|
||
|
|
|
||
|
|
- Parse error or syntax ambiguity
|
||
|
|
- Start in `src/lexer.rs` and `src/parser.rs`.
|
||
|
|
- Pretty-print or round-trip bug
|
||
|
|
- Start in `src/pretty.rs`, then `tests/unit_pretty.rs` and `tests/unit_parsing.rs`.
|
||
|
|
- Wrong theory or instance semantics
|
||
|
|
- Start in `src/elaborate/` and `src/core.rs`.
|
||
|
|
- Persistence or workspace bug
|
||
|
|
- Start in `src/repl.rs` and `src/store/`.
|
||
|
|
- Chase/query behavior bug
|
||
|
|
- Start in `src/query/chase.rs`, `src/query/compile.rs`, and related tests.
|
||
|
|
- GUI-only behavior bug
|
||
|
|
- Start in `src/gui/state.rs`, then `src/gui/panels/` or `src/gui/visualizations/`.
|
||
|
|
|
||
|
|
## Important Invariants
|
||
|
|
|
||
|
|
- `parser` should stay syntax-focused.
|
||
|
|
- Semantic decisions generally belong in elaboration, not parsing.
|
||
|
|
- `core` should remain GUI-free.
|
||
|
|
- GUI code should consume existing semantics, not define them.
|
||
|
|
- GUI must remain feature-gated.
|
||
|
|
- GUI code lives behind the `gui` feature.
|
||
|
|
- Persistence changes need extra care.
|
||
|
|
- `src/store/` and related runtime flows should preserve expected workspace behavior.
|
||
|
|
- Chase/query changes need explicit tests.
|
||
|
|
- These regressions are often subtle and easy to miss in manual review.
|
||
|
|
|
||
|
|
## Best Review Commands
|
||
|
|
|
||
|
|
Start narrow, then go broad.
|
||
|
|
|
||
|
|
### Core checks
|
||
|
|
|
||
|
|
```bash
|
||
|
|
cargo check
|
||
|
|
cargo test
|
||
|
|
```
|
||
|
|
|
||
|
|
### GUI checks
|
||
|
|
|
||
|
|
```bash
|
||
|
|
cargo check --features gui --bin geolog-gui
|
||
|
|
cargo test --features gui
|
||
|
|
```
|
||
|
|
|
||
|
|
### Focused checks
|
||
|
|
|
||
|
|
```bash
|
||
|
|
cargo test unit_parsing
|
||
|
|
cargo test unit_elaborate
|
||
|
|
cargo test unit_chase
|
||
|
|
cargo test unit_workspace
|
||
|
|
cargo test gui_
|
||
|
|
```
|
||
|
|
|
||
|
|
## Test File Guide
|
||
|
|
|
||
|
|
- `tests/unit_parsing.rs`
|
||
|
|
- Parser and syntax behavior.
|
||
|
|
- `tests/unit_pretty.rs`
|
||
|
|
- Pretty-printing and round-trip expectations.
|
||
|
|
- `tests/unit_elaborate.rs`
|
||
|
|
- Theory/instance elaboration semantics.
|
||
|
|
- `tests/unit_chase.rs`
|
||
|
|
- Chase behavior.
|
||
|
|
- `tests/unit_workspace.rs`
|
||
|
|
- Persistence/workspace behavior.
|
||
|
|
- `tests/examples_integration.rs`
|
||
|
|
- End-to-end examples.
|
||
|
|
- `tests/proptest_*.rs`
|
||
|
|
- Property-based coverage.
|
||
|
|
|
||
|
|
## What Usually Deserves Extra Scrutiny
|
||
|
|
|
||
|
|
- Changes that cross parser → elaboration boundaries.
|
||
|
|
- Changes in `src/core.rs` because they affect many layers.
|
||
|
|
- Anything in `src/query/chase.rs`, `src/query/backend.rs`, or `src/store/`.
|
||
|
|
- GUI changes that mutate runtime state rather than only presenting it.
|
||
|
|
- Feature-gated code that may compile less often than the default build.
|
||
|
|
|
||
|
|
## Useful Missing Context To Watch For
|
||
|
|
|
||
|
|
If a patch is hard to review, it often needs one of these:
|
||
|
|
|
||
|
|
- a smaller regression test,
|
||
|
|
- a more focused example in `examples/geolog/`,
|
||
|
|
- a note in `docs/ARCHITECTURE.md`,
|
||
|
|
- or a short comment explaining a non-obvious invariant.
|
||
|
|
|
||
|
|
## Reviewer Checklist
|
||
|
|
|
||
|
|
- Does the change live in the right layer?
|
||
|
|
- Are semantics changed intentionally, or just presentation?
|
||
|
|
- Is there a narrow test for the bug or behavior?
|
||
|
|
- If GUI code changed, is the same state transition safe from the REPL path too?
|
||
|
|
- If store/query/chase code changed, do existing tests still cover the affected behavior?
|
||
|
|
- Is any documentation now stale?
|