diff --git a/.gitignore b/.gitignore index 6b0015d6d..2988f9a9a 100644 --- a/.gitignore +++ b/.gitignore @@ -10,4 +10,5 @@ bisect.coverage .DS_Store .ai/ .auto-coder/ -.moonagent/ \ No newline at end of file +.moonagent/ +node_modules diff --git a/scripts/.gitignore b/scripts/.gitignore new file mode 100644 index 000000000..ea1472ec1 --- /dev/null +++ b/scripts/.gitignore @@ -0,0 +1 @@ +output/ diff --git a/scripts/CONCURRENCY_GUIDE.md b/scripts/CONCURRENCY_GUIDE.md new file mode 100644 index 000000000..453b1ba08 --- /dev/null +++ b/scripts/CONCURRENCY_GUIDE.md @@ -0,0 +1,144 @@ +# Code Review with Concurrency Control Example + +This document explains how the concurrency control works in the mbti review scripts. + +## How Concurrency Control Works + +The scripts use a **sliding window** approach to limit concurrent API calls: + +``` +Files to review: [F1, F2, F3, F4, F5, F6, F7, F8, F9, F10] +Concurrency limit: 5 + +Timeline: +───────────────────────────────────────────────────────── + +Time 0s: Start reviewing F1, F2, F3, F4, F5 + [F1▶] [F2▶] [F3▶] [F4▶] [F5▶] + +Time 2s: F1 completes, F6 starts + [F1✓] [F2▶] [F3▶] [F4▶] [F5▶] [F6▶] + +Time 3s: F3 completes, F7 starts + [F1✓] [F2▶] [F3✓] [F4▶] [F5▶] [F6▶] [F7▶] + +Time 4s: F2 and F4 complete, F8 and F9 start + [F1✓] [F2✓] [F3✓] [F4✓] [F5▶] [F6▶] [F7▶] [F8▶] [F9▶] + +Time 5s: F5 completes, F10 starts + [F1✓] [F2✓] [F3✓] [F4✓] [F5✓] [F6▶] [F7▶] [F8▶] [F9▶] [F10▶] + +Time 7s: Remaining files complete + [F1✓] [F2✓] [F3✓] [F4✓] [F5✓] [F6✓] [F7✓] [F8✓] [F9✓] [F10✓] +``` + +**Key Points:** +- Never more than 5 requests active at once +- New requests start immediately when a slot opens +- Total time depends on the slowest reviews in each batch +- Failed reviews don't block others + +## Implementation Details + +```javascript +async function processConcurrently(files, processor, concurrency) { + const results = []; + const executing = []; + + for (const file of files) { + // Start a new review + const promise = processor(file).then((result) => { + results.push(result); + // Remove from executing list when done + executing.splice(executing.indexOf(promise), 1); + return result; + }); + + executing.push(promise); + + // Wait if we've hit the concurrency limit + if (executing.length >= concurrency) { + await Promise.race(executing); // Wait for ANY to complete + } + } + + // Wait for all remaining to complete + await Promise.all(executing); + return results; +} +``` + +## Choosing the Right Concurrency + +### Concurrency = 1 (Sequential) +- **Pros:** Safest for rate limits, predictable timing +- **Cons:** Slowest +- **Use when:** You have strict rate limits or want to be very conservative + +### Concurrency = 5 (Default) +- **Pros:** Good balance of speed and safety +- **Cons:** None for most use cases +- **Use when:** Normal usage, recommended default + +### Concurrency = 10+ (Aggressive) +- **Pros:** Fastest possible +- **Cons:** May hit rate limits, higher chance of errors +- **Use when:** You have high quota and need results quickly + +## Example Usage + +```bash +# Conservative approach (avoid hitting rate limits) +node scripts/review-mbti-files.mjs --changed --concurrency=2 + +# Default balanced approach +node scripts/review-mbti-files.mjs --changed + +# Aggressive approach (if you have high API quota) +node scripts/review-mbti-files.mjs --all --concurrency=10 + +# Review just the bytes package +node scripts/review-mbti-files.mjs --files=bytes/pkg.generated.mbti + +# See progress in real-time +node scripts/review-mbti-files.mjs --changed --verbose +``` + +## Error Handling + +The scripts handle errors gracefully: +- Failed reviews are collected and reported at the end +- Failures don't stop other reviews +- You can retry failed files by passing them with `--files` + +Example output: +``` +✓ Reviewed 1/5: bytes/pkg.generated.mbti +✓ Reviewed 2/5: string/pkg.generated.mbti +✗ Failed 3/5: error/pkg.generated.mbti (API timeout) +✓ Reviewed 4/5: array/pkg.generated.mbti +✓ Reviewed 5/5: list/pkg.generated.mbti + +SUMMARY +═══════════════════════════════════════ +Total files: 5 +Successful: 4 +Failed: 1 +Duration: 12.5s +Concurrency: 5 + +❌ Failed reviews: + - error/pkg.generated.mbti: API timeout +``` + +## Performance Estimates + +Based on typical API response times (~2-3 seconds per review): + +| Files | Concurrency=1 | Concurrency=5 | Concurrency=10 | +|-------|---------------|---------------|----------------| +| 10 | ~25s | ~6s | ~3s | +| 30 | ~75s | ~18s | ~9s | +| 60 | ~150s (2.5m) | ~36s | ~18s | + +*Note: Actual times vary based on file size, API latency, and network conditions.* diff --git a/scripts/FIX_OBJECT_ISSUE.md b/scripts/FIX_OBJECT_ISSUE.md new file mode 100644 index 000000000..b843161f2 --- /dev/null +++ b/scripts/FIX_OBJECT_ISSUE.md @@ -0,0 +1,130 @@ +# Fix: [object Object] Issue - RESOLVED ✅ + +## Problem + +Review files were showing `[object Object]` instead of the actual review text because the Codex SDK returns an object, not a plain string. + +## Root Cause + +The Codex SDK's `thread.run()` method returns an object with this structure: + +```javascript +{ + "items": [ + { "id": "item_0", "type": "reasoning", "text": "..." }, + { "id": "item_1", "type": "agent_message", "text": "..." } + ], + "finalResponse": "The actual review text here", + "usage": { ... } +} +``` + +We were trying to save the entire object as a string, which JavaScript converts to `[object Object]`. + +## Solution + +Updated both scripts to extract the `finalResponse` property: + +```javascript +// Before (broken) +const result = await thread.run(prompt); +return { + review: result, // This is an object! + success: true, +}; + +// After (fixed) +const result = await thread.run(prompt); +const reviewText = result?.finalResponse || JSON.stringify(result, null, 2); +return { + review: reviewText, // Now it's a string! + success: true, +}; +``` + +## Files Fixed + +1. ✅ `scripts/review-mbti-files.mjs` - Updated to extract `finalResponse` +2. ✅ `scripts/list-mbti-files.mjs` - Updated to extract `finalResponse` + +## Cleanup Done + +Removed 7 broken review files that contained `[object Object]`: +- array.review.md +- bench.review.md +- bigint.review.md +- bool.review.md +- buffer.review.md +- byte.review.md +- char.review.md + +## Utility Scripts Created + +1. **`test-codex-response.mjs`** - Test to inspect Codex SDK response structure +2. **`test-review-extraction.mjs`** - Test to verify review extraction works +3. **`clean-broken-reviews.mjs`** - Utility to remove broken review files + +## Next Steps + +You can now re-run reviews and they will save properly: + +```bash +# Test with one file first +node scripts/review-mbti-files.mjs --files=bytes/pkg.generated.mbti + +# Verify it worked +cat scripts/output/bytes.review.md + +# If good, run for all your changes +node scripts/review-mbti-files.mjs --changed --concurrency=5 +``` + +## Expected Output + +Review files should now contain actual review text: + +```markdown +# Review: moonbitlang/core/bytes + +**File:** `bytes/pkg.generated.mbti` +**Date:** 2025-10-09T10:15:23.456Z +**Status:** ✓ Success + +--- + +The Bytes package has a well-designed API with clear separation between +immutable Bytes and view-based BytesView types. The interface provides +comprehensive functionality for byte manipulation... + +[More actual review content here] +``` + +Instead of: + +```markdown +--- + +[object Object] ❌ +``` + +## Testing + +Run the test script to verify: + +```bash +node scripts/test-review-extraction.mjs +``` + +This will: +1. Read the bool package interface +2. Send it to Codex for review +3. Show the extracted review text +4. Confirm the fix works + +## Summary + +- ✅ Issue identified: Codex SDK returns object with `finalResponse` property +- ✅ Scripts updated to extract `finalResponse` +- ✅ Broken files cleaned up +- ✅ Utility scripts created for testing and cleanup +- ✅ Ready to generate proper reviews diff --git a/scripts/LIST_MBTI_README.md b/scripts/LIST_MBTI_README.md new file mode 100644 index 000000000..55345db28 --- /dev/null +++ b/scripts/LIST_MBTI_README.md @@ -0,0 +1,164 @@ +# List MoonBit Generated Interface Files + +Scripts to list and review all `pkg.generated.mbti` files in the MoonBit Core project. + +## Available Scripts + +### Node.js Version (`list-mbti-files.mjs`) + +**Usage:** +```bash +node scripts/list-mbti-files.mjs [options] +``` + +**Options:** +- `--relative` - Show paths relative to project root (default) +- `--absolute` - Show absolute paths +- `--count` - Only show the count of files +- `--json` - Output as JSON array +- `--review` - Review files using Codex SDK (with concurrency control) +- `--concurrency=N` - Number of concurrent reviews when using --review (default: 5) + +**Examples:** +```bash +# List all files (relative paths) +node scripts/list-mbti-files.mjs + +# Count total files +node scripts/list-mbti-files.mjs --count + +# Get JSON output +node scripts/list-mbti-files.mjs --json + +# Review all files with concurrency limit of 5 (default) +node scripts/list-mbti-files.mjs --review + +# Review with custom concurrency limit +node scripts/list-mbti-files.mjs --review --concurrency=3 +``` + +### Dedicated Review Script (`review-mbti-files.mjs`) + +A standalone script specifically for reviewing mbti files with more options. + +**Usage:** +```bash +node scripts/review-mbti-files.mjs [options] +``` + +**Options:** +- `--all` - Review all mbti files (default) +- `--changed` - Only review changed files in git +- `--files=path1,path2` - Specific files to review (comma-separated) +- `--concurrency=N` - Number of concurrent reviews (default: 5) +- `--verbose` - Show detailed progress + +**Examples:** +```bash +# Review all files with default concurrency (5) +node scripts/review-mbti-files.mjs + +# Review only changed files +node scripts/review-mbti-files.mjs --changed + +# Review specific files +node scripts/review-mbti-files.mjs --files=bytes/pkg.generated.mbti,string/pkg.generated.mbti + +# Review with custom concurrency limit +node scripts/review-mbti-files.mjs --concurrency=3 --verbose +``` + +### Bash Version (`list-mbti-files.sh`) + +**Usage:** +```bash +./scripts/list-mbti-files.sh [options] +``` + +**Options:** +- `--count` - Only show the count of files +- `--absolute` - Show absolute paths (default is relative) +- `--help` - Show help message + +**Examples:** +```bash +# List all files (relative paths) +./scripts/list-mbti-files.sh + +# Count total files +./scripts/list-mbti-files.sh --count + +# Show absolute paths +./scripts/list-mbti-files.sh --absolute +``` + +## What are `pkg.generated.mbti` files? + +These files are automatically generated by the MoonBit build system (`moon info`) and contain the public interface definitions for each package. They specify: +- Exported functions and their signatures +- Public types and their methods +- Type aliases +- Trait implementations + +These files should be committed to version control as they document the public API of each package. + +## Common Use Cases + +**Review recently changed interfaces:** +```bash +node scripts/review-mbti-files.mjs --changed +``` + +**Review specific package:** +```bash +node scripts/review-mbti-files.mjs --files=bytes/pkg.generated.mbti +``` + +**Control API rate limiting:** +```bash +# Lower concurrency to avoid rate limits +node scripts/review-mbti-files.mjs --concurrency=2 + +# Higher concurrency for faster reviews (if quota allows) +node scripts/review-mbti-files.mjs --concurrency=10 +``` + +**Check if interfaces are up to date:** +```bash +moon info +git diff --name-only | grep pkg.generated.mbti +``` + +**Find packages that changed their public API:** +```bash +git diff main --name-only | grep pkg.generated.mbti +``` + +**List all packages in the project:** +```bash +node scripts/list-mbti-files.mjs | sed 's|/pkg.generated.mbti||' +``` + +**Count packages:** +```bash +node scripts/list-mbti-files.mjs --count +``` + +## Concurrency Control + +The review scripts implement a concurrency limit to avoid overwhelming the OpenAI API service. The default concurrency is **5**, which means: + +- Maximum 5 files are reviewed simultaneously +- New reviews start as soon as one completes +- Prevents API rate limiting and quota exhaustion +- Balances speed with resource usage + +**Adjusting concurrency:** +- Lower (1-3): More conservative, better for rate limits +- Default (5): Balanced performance +- Higher (8-10): Faster but may hit rate limits + +**Example timing (approximate):** +- 60 files @ concurrency=1: ~10 minutes +- 60 files @ concurrency=5: ~2-3 minutes +- 60 files @ concurrency=10: ~1-2 minutes (if quota allows) diff --git a/scripts/OUTPUT_FEATURE.md b/scripts/OUTPUT_FEATURE.md new file mode 100644 index 000000000..065ab8323 --- /dev/null +++ b/scripts/OUTPUT_FEATURE.md @@ -0,0 +1,243 @@ +# Review Output Feature - Summary + +## What Changed + +Both review scripts now **automatically save** each review to individual Markdown files in the `scripts/output/` directory. + +## File Structure + +``` +scripts/ +├── list-mbti-files.mjs # Updated with file saving +├── review-mbti-files.mjs # Updated with file saving +└── output/ + ├── README.md # Documentation for output directory + ├── bytes.review.md # Example: bytes package review + ├── string.review.md # Example: string package review + ├── array.review.md # Example: array package review + └── ... # One file per package +``` + +## File Naming + +Reviews are named after their package path: +- `bytes/pkg.generated.mbti` → `bytes.review.md` +- `string/pkg.generated.mbti` → `string.review.md` +- `immut/array/pkg.generated.mbti` → `immut_array.review.md` + +## Progress Output + +Now shows where each review is saved: + +```bash +✓ [1/8] bytes/pkg.generated.mbti (2.34s, ETA: 16s) → scripts/output/bytes.review.md +✓ [2/8] string/pkg.generated.mbti (2.87s, ETA: 15s) → scripts/output/string.review.md +✗ [3/8] error/pkg.generated.mbti (0.52s, ETA: 12s) → scripts/output/error.review.md +``` + +Even failed reviews are saved with error information. + +## Review File Format + +Each `.review.md` file contains: + +```markdown +# Review: moonbitlang/core/bytes + +**File:** `bytes/pkg.generated.mbti` +**Date:** 2025-10-09T12:34:56.789Z +**Status:** ✓ Success + +--- + +[Codex review content with API assessment, issues, and suggestions] +``` + +## Benefits + +### 1. Persistent Storage +- Reviews don't disappear after script finishes +- Can reference them later +- Build up a knowledge base over time + +### 2. Easy Navigation +- One file per package +- Simple filenames matching package names +- Can open directly in any editor + +### 3. Searchable +```bash +# Find all mentions of "deprecated" +grep -r "deprecated" scripts/output/ + +# Find reviews with potential issues +grep -r "potential issue" scripts/output/ + +# Search for specific package +grep -l "bytes" scripts/output/*.md +``` + +### 4. Diffable +```bash +# Compare two reviews +diff scripts/output/bytes.review.md scripts/output/string.review.md + +# Track changes over time (if committed) +git diff HEAD~1 scripts/output/bytes.review.md +``` + +### 5. Shareable +```bash +# Send specific review to teammate +cat scripts/output/bytes.review.md | pbcopy + +# Attach to GitHub issue +gh issue create --body-file scripts/output/bytes.review.md +``` + +### 6. Batch Processing +```bash +# Extract all suggestions +grep -A 5 "Suggestions:" scripts/output/*.md > all_suggestions.txt + +# Count reviews with issues +grep -l "Potential Issues:" scripts/output/*.md | wc -l + +# Create summary of all ratings +grep "Rating:" scripts/output/*.md +``` + +## Workflow Examples + +### Review → Fix → Verify +```bash +# 1. Run review +node scripts/review-mbti-files.mjs --files=bytes/pkg.generated.mbti + +# 2. Read and implement suggestions +code scripts/output/bytes.review.md + +# 3. Make changes to bytes package +# ... + +# 4. Re-run review to verify improvements +node scripts/review-mbti-files.mjs --files=bytes/pkg.generated.mbti + +# 5. Compare before/after (if reviews committed) +git diff scripts/output/bytes.review.md +``` + +### Bulk Review for Documentation +```bash +# 1. Review all packages +node scripts/review-mbti-files.mjs --all --concurrency=5 + +# 2. Extract all API design issues +grep -B 2 -A 5 "API Design" scripts/output/*.md > api_issues.txt + +# 3. Create tracking issues from reviews +for file in scripts/output/*.md; do + package=$(basename "$file" .review.md) + echo "Creating issue for $package..." + # gh issue create --title "API review: $package" --body-file "$file" +done +``` + +### PR Review Integration +```bash +# 1. Review only changed files +node scripts/review-mbti-files.mjs --changed + +# 2. Check which reviews have concerns +grep -l "Potential Issues:" scripts/output/*.md + +# 3. Add review comments to PR +for file in scripts/output/*.md; do + echo "## $(basename "$file")" + cat "$file" + echo "" +done > pr_review.md +``` + +## Git Integration + +By default, `output/` is gitignored. To track reviews: + +```bash +# Remove from gitignore +sed -i '' '/output\//d' scripts/.gitignore + +# Commit reviews +git add scripts/output/ +git commit -m "Add API reviews for bytes and string packages" + +# Track review changes over time +git log -p scripts/output/bytes.review.md +``` + +## Cleanup + +```bash +# Remove all reviews +rm -rf scripts/output/*.review.md + +# Remove specific review +rm scripts/output/bytes.review.md + +# Remove old reviews (older than 7 days) +find scripts/output -name "*.review.md" -mtime +7 -delete + +# Keep only latest N reviews +ls -t scripts/output/*.review.md | tail -n +11 | xargs rm +``` + +## Summary of Changes + +### Modified Files: +1. `scripts/list-mbti-files.mjs` - Added `saveReviewToFile()` function +2. `scripts/review-mbti-files.mjs` - Added `saveReviewToFile()` function + +### New Files: +1. `scripts/output/README.md` - Documentation for output directory +2. `scripts/output/bytes.review.example.md` - Example review file +3. `scripts/.gitignore` - Added output/ to gitignore + +### Key Functions Added: +```javascript +async function saveReviewToFile(result) { + // Creates output directory if needed + // Generates filename from package path + // Writes formatted Markdown with review + // Returns path to saved file +} +``` + +### Progress Logging Updated: +```javascript +// Before +console.log(`✓ [1/8] bytes/pkg.generated.mbti (2.34s, ETA: 16s)`) + +// After +console.log(`✓ [1/8] bytes/pkg.generated.mbti (2.34s, ETA: 16s) → scripts/output/bytes.review.md`) +``` + +## Next Steps + +1. **Run a test review** (with 1-2 files to avoid API charges): + ```bash + node scripts/review-mbti-files.mjs --files=bytes/pkg.generated.mbti + cat scripts/output/bytes.review.md + ``` + +2. **Review changed files** from your PR: + ```bash + node scripts/review-mbti-files.mjs --changed --concurrency=3 + ``` + +3. **Browse and address issues** in saved reviews: + ```bash + ls scripts/output/ + code scripts/output/ + ``` + +4. **Decide on git tracking**: Keep reviews gitignored or commit them for history. diff --git a/scripts/PROGRESS_LOGGING.md b/scripts/PROGRESS_LOGGING.md new file mode 100644 index 000000000..25fb5037b --- /dev/null +++ b/scripts/PROGRESS_LOGGING.md @@ -0,0 +1,116 @@ +# Progress Logging Example + +When you run the review scripts, you'll now see detailed progress information: + +## Example Output + +```bash +$ node scripts/review-mbti-files.mjs --changed --concurrency=5 + +🔍 Starting mbti file review... + +Found 8 changed mbti file(s) +Using concurrency limit: 5 + +✓ [1/8] bytes/pkg.generated.mbti (2.34s, ETA: 16s) +✓ [2/8] string/pkg.generated.mbti (2.87s, ETA: 15s) +✓ [3/8] array/pkg.generated.mbti (1.95s, ETA: 12s) +✗ [4/8] error/pkg.generated.mbti (0.52s, ETA: 9s) +✓ [5/8] list/pkg.generated.mbti (3.12s, ETA: 7s) +✓ [6/8] hashmap/pkg.generated.mbti (2.45s, ETA: 5s) +✓ [7/8] buffer/pkg.generated.mbti (2.78s, ETA: 2s) +✓ [8/8] json/pkg.generated.mbti (2.21s, ETA: 0s) + +════════════════════════════════════════════════════════════════════════════════ +REVIEW RESULTS +════════════════════════════════════════════════════════════════════════════════ + +──────────────────────────────────────────────────────────────────────────────── +📦 moonbitlang/core/bytes (bytes/pkg.generated.mbti) +──────────────────────────────────────────────────────────────────────────────── + +The API design is well-structured with clear separation between Bytes and +BytesView. The repeat function is a nice addition... + +[... rest of review ...] + +════════════════════════════════════════════════════════════════════════════════ +SUMMARY +════════════════════════════════════════════════════════════════════════════════ +Total files: 8 +Successful: 7 +Failed: 1 +Duration: 18.24s +Concurrency: 5 + +❌ Failed reviews: + - error/pkg.generated.mbti: API timeout +``` + +## Progress Information Explained + +Each line shows: + +``` +✓ [3/8] array/pkg.generated.mbti (1.95s, ETA: 12s) +│ │ │ │ │ +│ │ │ │ └─ Estimated time to completion +│ │ │ └─ Time taken for this file +│ │ └─ File path (relative to project root) +│ └─ Progress counter (completed/total) +└─ Status (✓ success, ✗ failure) +``` + +### Status Indicators: +- **✓** Green checkmark - Review completed successfully +- **✗** Red X - Review failed (error, timeout, etc.) + +### Timing Information: +- **File time** - How long this specific file took to review +- **ETA** - Estimated time remaining based on average review time + - Updates dynamically as reviews complete + - Gets more accurate as more files are processed + +## Benefits of Progress Logging + +1. **Visibility** - See which files are being reviewed in real-time +2. **Performance** - Identify slow reviews that might need optimization +3. **Errors** - Immediately see which files failed (without waiting for summary) +4. **Planning** - ETA helps you decide if you have time to wait or should reduce scope +5. **Debugging** - File times help identify API performance issues + +## Example Scenarios + +### Fast reviews (small files): +``` +✓ [1/10] bool/pkg.generated.mbti (0.89s, ETA: 8s) +✓ [2/10] byte/pkg.generated.mbti (0.76s, ETA: 6s) +✓ [3/10] unit/pkg.generated.mbti (0.81s, ETA: 6s) +``` + +### Slow reviews (large files or API delays): +``` +✓ [4/10] builtin/pkg.generated.mbti (5.23s, ETA: 24s) +✓ [5/10] string/pkg.generated.mbti (4.87s, ETA: 19s) +``` + +### Mixed success/failure: +``` +✓ [6/10] array/pkg.generated.mbti (2.34s, ETA: 10s) +✗ [7/10] error/pkg.generated.mbti (0.21s, ETA: 7s) # Failed quickly +✓ [8/10] json/pkg.generated.mbti (3.12s, ETA: 6s) +``` + +## Concurrency in Action + +With `--concurrency=5`, you'll see reviews completing out of order as different files finish at different times: + +``` +✓ [1/10] file1 (2.1s, ETA: 18s) # Started first +✓ [3/10] file3 (1.8s, ETA: 14s) # Finished before file2! +✓ [2/10] file2 (2.9s, ETA: 14s) # Took longer +✓ [4/10] file4 (2.2s, ETA: 12s) +✓ [5/10] file5 (1.5s, ETA: 9s) +``` + +This is normal and shows the concurrency is working - files don't wait for previous files to finish. diff --git a/scripts/QUICK_START.md b/scripts/QUICK_START.md new file mode 100644 index 000000000..b50d79d65 --- /dev/null +++ b/scripts/QUICK_START.md @@ -0,0 +1,128 @@ +# MBTI Review Scripts - Quick Start + +## TL;DR + +Two ways to review `pkg.generated.mbti` files with Codex SDK. **All reviews are automatically saved to `scripts/output/` directory.** + +### Option 1: Using the integrated list script +```bash +# Review all files with concurrency limit of 5 (won't spam API) +node scripts/list-mbti-files.mjs --review + +# Review with lower concurrency (more conservative) +node scripts/list-mbti-files.mjs --review --concurrency=3 +``` + +### Option 2: Using the dedicated review script +```bash +# Review only changed files +node scripts/review-mbti-files.mjs --changed + +# Review specific files +node scripts/review-mbti-files.mjs --files=bytes/pkg.generated.mbti,string/pkg.generated.mbti + +# Review all with custom concurrency +node scripts/review-mbti-files.mjs --all --concurrency=3 --verbose +``` + +## Key Features + +✅ **Concurrency Control**: Default limit of 5 prevents API spam +✅ **Smart Queueing**: New reviews start as soon as slots open +✅ **Progress Tracking**: See which files are being reviewed +✅ **Error Handling**: Failed reviews don't block others +✅ **Flexible Targeting**: Review all, changed, or specific files +✅ **Persistent Storage**: Reviews saved to `scripts/output/` directory +✅ **Individual Files**: One .md file per package for easy navigation + +## Review Output + +Reviews are saved as Markdown files in `scripts/output/`: + +``` +scripts/output/ +├── bytes.review.md +├── string.review.md +├── array.review.md +└── ... +``` + +**Example progress output:** +``` +✓ [1/8] bytes/pkg.generated.mbti (2.34s, ETA: 16s) → scripts/output/bytes.review.md +✓ [2/8] string/pkg.generated.mbti (2.87s, ETA: 15s) → scripts/output/string.review.md +``` + +## Files Created + +1. **`list-mbti-files.mjs`** - List AND review mbti files (integrated) +2. **`review-mbti-files.mjs`** - Dedicated review script with more options +3. **`LIST_MBTI_README.md`** - Full documentation +4. **`CONCURRENCY_GUIDE.md`** - Detailed concurrency explanation + +## Common Workflows + +**Review recently changed interfaces:** +```bash +node scripts/review-mbti-files.mjs --changed +# Reviews saved to scripts/output/ +``` + +**Review specific package you're working on:** +```bash +node scripts/review-mbti-files.mjs --files=bytes/pkg.generated.mbti +# Review saved to scripts/output/bytes.review.md +``` + +**Browse saved reviews:** +```bash +# List all reviews +ls scripts/output/ + +# Open specific review +code scripts/output/bytes.review.md + +# Search for issues across all reviews +grep -r "potential issue" scripts/output/ +``` + +**Full codebase review (use with caution on quota):** +```bash +node scripts/review-mbti-files.mjs --all --concurrency=3 +# All 60 reviews saved to scripts/output/ +``` + +**List files without reviewing:** +```bash +node scripts/list-mbti-files.mjs +node scripts/list-mbti-files.mjs --count +node scripts/list-mbti-files.mjs --json +``` + +## Why Concurrency Control? + +Without concurrency limits, reviewing 60 files would fire 60 simultaneous API requests: +- ❌ May hit rate limits +- ❌ May exceed quota quickly +- ❌ May cause API errors +- ❌ Wasteful of resources + +With concurrency=5: +- ✅ Max 5 requests at once +- ✅ Respects rate limits +- ✅ Predictable quota usage +- ✅ Still reasonably fast + +## Configuration + +Default concurrency is **5**, which is a good balance. Adjust based on your needs: + +```bash +--concurrency=1 # Very conservative (sequential) +--concurrency=3 # Conservative +--concurrency=5 # Balanced (default) +--concurrency=8 # Aggressive +--concurrency=10 # Very aggressive (may hit limits) +``` + +See `CONCURRENCY_GUIDE.md` for detailed explanation and performance estimates. diff --git a/scripts/clean-broken-reviews.mjs b/scripts/clean-broken-reviews.mjs new file mode 100755 index 000000000..077456edf --- /dev/null +++ b/scripts/clean-broken-reviews.mjs @@ -0,0 +1,46 @@ +#!/usr/bin/env node + +/** + * Utility to clean up broken review files that contain "[object Object]" + */ + +import { readdir, readFile, unlink } from "fs/promises"; +import { join } from "path"; +import { fileURLToPath } from "url"; +import { dirname } from "path"; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); +const outputDir = join(__dirname, "output"); + +async function cleanBrokenReviews() { + console.log("🧹 Cleaning up broken review files...\n"); + + try { + const files = await readdir(outputDir); + const reviewFiles = files.filter(f => f.endsWith('.review.md') && !f.includes('example')); + + let removed = 0; + + for (const file of reviewFiles) { + const filePath = join(outputDir, file); + const content = await readFile(filePath, 'utf-8'); + + // Check if file contains [object Object] + if (content.includes('[object Object]')) { + console.log(` Removing: ${file}`); + await unlink(filePath); + removed++; + } + } + + console.log(`\n✓ Removed ${removed} broken review file(s)`); + console.log(` Remaining: ${reviewFiles.length - removed} file(s)`); + console.log('\n💡 You can now re-run the review script to generate proper reviews.'); + + } catch (error) { + console.error("Error:", error.message); + } +} + +cleanBrokenReviews(); diff --git a/scripts/index.mjs b/scripts/index.mjs new file mode 100644 index 000000000..404e22a5f --- /dev/null +++ b/scripts/index.mjs @@ -0,0 +1,9 @@ +import { Codex } from "@openai/codex-sdk"; + +const codex = new Codex(); +const thread = codex.startThread({skipGitRepoCheck: true}); +// const result = await thread.run( +// "Make a plan to diagnose and fix the CI failures" +// ); + +// console.log(result); \ No newline at end of file diff --git a/scripts/list-mbti-files.mjs b/scripts/list-mbti-files.mjs new file mode 100755 index 000000000..525407517 --- /dev/null +++ b/scripts/list-mbti-files.mjs @@ -0,0 +1,288 @@ +#!/usr/bin/env node + +/** + * Script to list all pkg.generated.mbti files in the project + * Usage: node scripts/list-mbti-files.mjs [options] + * + * Options: + * --relative Show paths relative to project root (default) + * --absolute Show absolute paths + * --count Only show the count of files + * --json Output as JSON array + * --review Review files using Codex SDK (with concurrency control) + * --concurrency Number of concurrent reviews when using --review (default: 5) + */ + +import { fileURLToPath } from "url"; +import { dirname, join, relative } from "path"; +import { readdir, readFile, writeFile, mkdir } from "fs/promises"; +import { Codex } from "@openai/codex-sdk"; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); +const projectRoot = join(__dirname, ".."); +const outputDir = join(__dirname, "output"); + +// Parse command line arguments +const args = process.argv.slice(2); +const options = { + absolute: args.includes("--absolute"), + count: args.includes("--count"), + json: args.includes("--json"), + relative: !args.includes("--absolute"), // default is relative + review: args.includes("--review"), + concurrency: parseInt( + args.find((a) => a.startsWith("--concurrency="))?.split("=")[1] || "5" + ), +}; + +/** + * Recursively find all pkg.generated.mbti files + */ +async function findMbtiFiles(dir, results = []) { + try { + const entries = await readdir(dir, { withFileTypes: true }); + + for (const entry of entries) { + const fullPath = join(dir, entry.name); + + // Skip node_modules, target, and hidden directories + if (entry.isDirectory()) { + if ( + !entry.name.startsWith(".") && + entry.name !== "node_modules" && + entry.name !== "target" + ) { + await findMbtiFiles(fullPath, results); + } + } else if (entry.name === "pkg.generated.mbti") { + results.push(fullPath); + } + } + } catch (error) { + // Skip directories we can't read + if (error.code !== "EACCES" && error.code !== "EPERM") { + console.error(`Error reading ${dir}:`, error.message); + } + } + + return results; +} + +/** + * Save review result to file + */ +async function saveReviewToFile(result) { + try { + // Create output directory if it doesn't exist + await mkdir(outputDir, { recursive: true }); + + // Generate filename from package path + // e.g., "bytes/pkg.generated.mbti" -> "bytes.review.md" + const fileName = result.file + .replace(/\/pkg\.generated\.mbti$/, "") + .replace(/\//g, "_") + ".review.md"; + + const outputPath = join(outputDir, fileName); + + // Format the review content + const content = `# Review: ${result.package} + +**File:** \`${result.file}\` +**Date:** ${new Date().toISOString()} +**Status:** ${result.success ? "✓ Success" : "✗ Failed"} + +${result.success ? "---\n\n" + result.review : `**Error:** ${result.error}`} +`; + + await writeFile(outputPath, content, "utf-8"); + return outputPath; + } catch (error) { + console.error(`Failed to save review for ${result.file}:`, error.message); + return null; + } +} + +/** + * Review a single mbti file using Codex + */ +async function reviewMbtiFile(filePath, codex) { + const relativePath = relative(projectRoot, filePath); + + try { + const content = await readFile(filePath, "utf-8"); + const packageName = content.match(/package "(.+?)"/)?.[1] || "unknown"; + + const thread = codex.startThread({ skipGitRepoCheck: true }); + + const prompt = `Review this MoonBit package interface file (${relativePath}): + +\`\`\`moonbit +${content} +\`\`\` + +Please provide: +1. A brief assessment of the API design +2. Any potential issues or inconsistencies +3. Suggestions for improvement (if any) + +Keep the review concise and focused on the public API surface.`; + + const result = await thread.run(prompt); + + // Extract text from Codex SDK result - it returns an object with finalResponse + const reviewText = result?.finalResponse || JSON.stringify(result, null, 2); + + return { + file: relativePath, + package: packageName, + review: reviewText, + success: true, + }; + } catch (error) { + return { + file: relativePath, + package: "unknown", + review: null, + success: false, + error: error.message, + }; + } +} + +/** + * Process files with concurrency limit + */ +async function processConcurrently(files, processor, concurrency) { + const results = []; + const executing = []; + const startTime = Date.now(); + + for (const file of files) { + const fileStartTime = Date.now(); + + const promise = processor(file).then(async (result) => { + results.push(result); + executing.splice(executing.indexOf(promise), 1); + + // Save review to file + const outputPath = await saveReviewToFile(result); + + // Calculate timing + const fileTime = ((Date.now() - fileStartTime) / 1000).toFixed(2); + const totalTime = ((Date.now() - startTime) / 1000).toFixed(1); + const avgTime = (totalTime / results.length).toFixed(2); + const eta = ((files.length - results.length) * avgTime).toFixed(0); + + // Show progress with status indicator + const status = result.success ? "✓" : "✗"; + const savedMsg = outputPath ? ` → ${relative(projectRoot, outputPath)}` : ""; + console.log( + `${status} [${results.length}/${files.length}] ${result.file} (${fileTime}s, ETA: ${eta}s)${savedMsg}` + ); + + return result; + }); + + executing.push(promise); + + if (executing.length >= concurrency) { + await Promise.race(executing); + } + } + + await Promise.all(executing); + return results; +} + +/** + * Perform code review on mbti files + */ +async function performReview(files) { + console.log(`\n🔍 Reviewing ${files.length} mbti file(s)...`); + console.log(`Concurrency limit: ${options.concurrency}\n`); + + const codex = new Codex(); + const startTime = Date.now(); + + const results = await processConcurrently( + files, + (file) => reviewMbtiFile(file, codex), + options.concurrency + ); + + const duration = ((Date.now() - startTime) / 1000).toFixed(2); + + // Display results + console.log("\n" + "=".repeat(80)); + console.log("REVIEW RESULTS"); + console.log("=".repeat(80) + "\n"); + + const successful = results.filter((r) => r.success); + const failed = results.filter((r) => !r.success); + + successful.forEach((result) => { + console.log(`\n${"─".repeat(80)}`); + console.log(`📦 ${result.package} (${result.file})`); + console.log(`${"─".repeat(80)}\n`); + console.log(result.review); + }); + + // Summary + console.log("\n" + "=".repeat(80)); + console.log("SUMMARY"); + console.log("=".repeat(80)); + console.log(`Total files: ${results.length}`); + console.log(`Successful: ${successful.length}`); + console.log(`Failed: ${failed.length}`); + console.log(`Duration: ${duration}s`); + console.log(`Concurrency: ${options.concurrency}`); + console.log(`Output directory: ${relative(projectRoot, outputDir)}`); + + if (failed.length > 0) { + console.log("\n❌ Failed reviews:"); + failed.forEach((f) => { + console.log(` - ${f.file}: ${f.error}`); + }); + } + + console.log(`\n💾 Reviews saved to: ${outputDir}`); +} + +/** + * Main function + */ +async function main() { + const files = await findMbtiFiles(projectRoot); + + // Sort files alphabetically + files.sort(); + + // If review mode, perform reviews instead of listing + if (options.review) { + await performReview(files); + return; + } + + if (options.count) { + console.log(files.length); + return; + } + + // Convert to relative paths if needed + const displayFiles = options.absolute + ? files + : files.map((f) => relative(projectRoot, f)); + + if (options.json) { + console.log(JSON.stringify(displayFiles, null, 2)); + } else { + displayFiles.forEach((f) => console.log(f)); + } +} + +// Run the script +main().catch((error) => { + console.error("Error:", error.message); + process.exit(1); +}); diff --git a/scripts/list-mbti-files.sh b/scripts/list-mbti-files.sh new file mode 100755 index 000000000..d7a00b259 --- /dev/null +++ b/scripts/list-mbti-files.sh @@ -0,0 +1,59 @@ +#!/usr/bin/env bash + +# Script to list all pkg.generated.mbti files in the project +# Usage: ./scripts/list-mbti-files.sh [options] +# +# Options: +# --count Only show the count of files +# --absolute Show absolute paths (default is relative) + +set -e + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" + +# Parse arguments +COUNT_ONLY=false +ABSOLUTE=false + +for arg in "$@"; do + case $arg in + --count) + COUNT_ONLY=true + ;; + --absolute) + ABSOLUTE=true + ;; + --help|-h) + echo "Usage: $0 [options]" + echo "" + echo "Options:" + echo " --count Only show the count of files" + echo " --absolute Show absolute paths (default is relative)" + echo " --help Show this help message" + exit 0 + ;; + *) + echo "Unknown option: $arg" + echo "Use --help for usage information" + exit 1 + ;; + esac +done + +cd "$PROJECT_ROOT" + +# Find all pkg.generated.mbti files, excluding node_modules and target directories +FILES=$(find . -type f -name 'pkg.generated.mbti' | \ + grep -v '/node_modules/' | \ + grep -v '/target/' | \ + grep -v '/\.' | \ + sort) + +if [ "$COUNT_ONLY" = true ]; then + echo "$FILES" | grep -c 'pkg.generated.mbti' +elif [ "$ABSOLUTE" = true ]; then + echo "$FILES" | sed "s|^\.|$PROJECT_ROOT|" +else + echo "$FILES" | sed 's|^\./||' +fi diff --git a/scripts/output/README.md b/scripts/output/README.md new file mode 100644 index 000000000..40bbf6567 --- /dev/null +++ b/scripts/output/README.md @@ -0,0 +1,123 @@ +# Review Output Directory + +This directory contains the saved review outputs for each `pkg.generated.mbti` file. + +## Directory Structure + +Each review is saved as a separate Markdown file named after the package: + +``` +output/ +├── bytes.review.md +├── string.review.md +├── array.review.md +├── builtin.review.md +├── immut_array.review.md +└── ... +``` + +## File Naming Convention + +The file names are derived from the package path: +- `bytes/pkg.generated.mbti` → `bytes.review.md` +- `string/pkg.generated.mbti` → `string.review.md` +- `immut/array/pkg.generated.mbti` → `immut_array.review.md` + +Slashes in paths are replaced with underscores. + +## Review File Format + +Each review file contains: + +```markdown +# Review: [package name] + +**File:** `[path to mbti file]` +**Date:** [ISO timestamp] +**Status:** ✓ Success / ✗ Failed + +--- + +[Review content from Codex] +``` + +## Usage Examples + +### Browse reviews in your editor +```bash +# Open all reviews +cd scripts/output +ls *.review.md + +# Open specific review +code bytes.review.md +``` + +### Search across reviews +```bash +# Find reviews mentioning specific terms +grep -r "potential issue" scripts/output/ + +# Find all deprecation warnings +grep -r "deprecated" scripts/output/ + +# Count total lines of feedback +wc -l scripts/output/*.review.md +``` + +### Compare reviews +```bash +# Compare two package reviews +diff scripts/output/bytes.review.md scripts/output/string.review.md +``` + +### Filter by date +```bash +# Find reviews from today +grep -l "$(date +%Y-%m-%d)" scripts/output/*.review.md +``` + +## Benefits + +✅ **Persistent storage** - Reviews are saved and can be referenced later +✅ **Easy navigation** - One file per package, easy to find +✅ **Searchable** - Use grep, ripgrep, or your editor's search +✅ **Diffable** - Track how reviews change over time +✅ **Shareable** - Send specific reviews to team members +✅ **Version control ready** - Can be committed if desired + +## Cleanup + +To remove old reviews: + +```bash +# Remove all reviews +rm -rf scripts/output/ + +# Remove specific review +rm scripts/output/bytes.review.md + +# Remove reviews older than 7 days +find scripts/output -name "*.review.md" -mtime +7 -delete +``` + +## Integration with Git + +The output directory is gitignored by default. If you want to commit reviews: + +```bash +# Remove from gitignore +nano scripts/.gitignore # Remove the output/ line + +# Commit reviews +git add scripts/output/ +git commit -m "Add API review for bytes package" +``` + +## Tips + +1. **Review one by one**: Open each file in your editor and address issues +2. **Create issues**: Copy specific feedback into GitHub issues +3. **Track improvements**: Re-run reviews after changes and compare +4. **Share with team**: Send relevant review files to package maintainers +5. **Aggregate insights**: Use grep to find common issues across packages diff --git a/scripts/output/array.review.md b/scripts/output/array.review.md new file mode 100644 index 000000000..1127d3006 --- /dev/null +++ b/scripts/output/array.review.md @@ -0,0 +1,22 @@ +# Review: moonbitlang/core/array + +**File:** `array/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:39.902Z +**Status:** ✓ Success + +--- + +**API Assessment** +- Surface covers the three array flavours comprehensively; higher-order helpers (`fold`, `map`, `rev_fold`) are exposed consistently and optional `raise?` effects make side-effects explicit. +- Byte-oriented unsafe extractors appear for all container flavours, which keeps low-level API uniform. +- Top-level `zip_with` complements the method-level zipping APIs, giving both functional and object-style entry points. + +**Issues** +- `FixedArray::join` is limited to `Self[String] -> String`, while `Array::join` leverages `@string.ToStringView`; the trait-based form is more flexible, so the mismatch between `array/pkg.generated.mbti:28` and `array/pkg.generated.mbti:62` feels arbitrary. +- `FixedArray::from_iter(Iter[T]) -> Self[T]` (`array/pkg.generated.mbti:26`) may surprise users because the iterator length must match the fixed array size; the contract is unclear from the signature. +- `Array::zip_to_iter2` (`array/pkg.generated.mbti:79`) names and shapes its iterator differently from `ArrayView::iter2` (`array/pkg.generated.mbti:91`), which returns `(index, value)`; the API symmetry is hard to discern. + +**Suggestions** +1. Align `FixedArray::join` with the trait-based signature used by `Array::join`, or document why strings-only is required (`array/pkg.generated.mbti:28`, `array/pkg.generated.mbti:62`). +2. Clarify `FixedArray::from_iter`’s expectations (length check, padding, truncation) via name or docs, or expose a safer `try_from_iter` that reports mismatches (`array/pkg.generated.mbti:26`). +3. Consider harmonising iterator-returning names—either rename `Array::zip_to_iter2` or offer `Array::iter2`/`Array::enumerate` to match the `ArrayView` vocabulary (`array/pkg.generated.mbti:79`, `array/pkg.generated.mbti:91`). diff --git a/scripts/output/bench.review.md b/scripts/output/bench.review.md new file mode 100644 index 000000000..83fa845b2 --- /dev/null +++ b/scripts/output/bench.review.md @@ -0,0 +1,20 @@ +# Review: moonbitlang/core/bench + +**File:** `bench/pkg.generated.mbti` +**Date:** 2025-10-09T09:56:45.314Z +**Status:** ✓ Success + +--- + +**API Design** +- The surface centers on a mutable runner `T` with helper free functions, giving a simple entry point for ad‑hoc (`single_bench`) and aggregated (`T::bench`) runs (`bench/pkg.generated.mbti:5`, `bench/pkg.generated.mbti:11`). + +**Issues** +- `monotonic_clock_start()` returns `Timestamp`, but `monotonic_clock_end(Timestamp) -> Double` leaks a bare floating-point duration, which is type inconsistent and risks precision/units confusion (`bench/pkg.generated.mbti:5`). +- `Summary` is exposed yet only convertible to JSON, while `T::dump_summaries()` yields a `String`; callers must parse JSON despite `Summary` existing (`bench/pkg.generated.mbti:13`, `bench/pkg.generated.mbti:17`). +- `T::keep(Self, Any) -> Unit` is effectively untyped and hides what can be stored, making misuse easy and hampering discoverability (`bench/pkg.generated.mbti:18`). + +**Suggestions** +- Return a dedicated duration type (or at least `Timestamp`) from `monotonic_clock_end`, or supply helpers that wrap both start/end and surface a strongly-typed duration (`bench/pkg.generated.mbti:5`). +- Let `T::dump_summaries()` return structured data—e.g., `[Summary]`—and/or add accessor methods on `Summary` so JSON parsing is optional (`bench/pkg.generated.mbti:13`, `bench/pkg.generated.mbti:17`). +- Replace `T::keep(Self, Any)` with a typed alternative (perhaps `Summary` or a generic parameter) to convey intent and improve safety (`bench/pkg.generated.mbti:18`). diff --git a/scripts/output/bigint.review.md b/scripts/output/bigint.review.md new file mode 100644 index 000000000..44f551435 --- /dev/null +++ b/scripts/output/bigint.review.md @@ -0,0 +1,20 @@ +# Review: moonbitlang/core/bigint + +**File:** `bigint/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:09.323Z +**Status:** ✓ Success + +--- + +**Assessment** +- Public surface offers comprehensive construction and conversion helpers plus standard arithmetic/bitwise traits, giving a predictable BigInt API (`bigint/pkg.generated.mbti:16`, `bigint/pkg.generated.mbti:22`, `bigint/pkg.generated.mbti:46`). + +**Issues** +- Text/hex parsers return plain `Self`, so callers have no error channel for malformed input; that likely means hidden panics or sentinel values (`bigint/pkg.generated.mbti:22`, `bigint/pkg.generated.mbti:26`). +- Deprecation markers leave both `lsl` and `shl`/`shr` methods exported alongside the operator traits, which keeps legacy names visible and risks accidental reuse (`bigint/pkg.generated.mbti:30`, `bigint/pkg.generated.mbti:33`, `bigint/pkg.generated.mbti:35`, `bigint/pkg.generated.mbti:58`, `bigint/pkg.generated.mbti:60`). +- Comparison helpers exist for signed integers only, so unsigned callers must round-trip through `BigInt`, which is inconsistent with the provided `from_uint*` constructors (`bigint/pkg.generated.mbti:17`, `bigint/pkg.generated.mbti:28`, `bigint/pkg.generated.mbti:45`). + +**Suggestions** +- Add fallible `Result`-returning variants (or document panic behavior) for string/hex parsing to make failure modes explicit (`bigint/pkg.generated.mbti:22`, `bigint/pkg.generated.mbti:26`). +- Move deprecated shift helpers into a dedicated `deprecated.mbt` (or hide them behind feature flags) to steer users toward the trait-based operators while still supporting legacy code (`bigint/pkg.generated.mbti:30`, `bigint/pkg.generated.mbti:33`, `bigint/pkg.generated.mbti:35`). +- Consider adding unsigned comparison helpers (e.g., `compare_uint`, `equal_uint`) for symmetry with existing constructors and to avoid wasteful conversions (`bigint/pkg.generated.mbti:27`, `bigint/pkg.generated.mbti:43`, `bigint/pkg.generated.mbti:45`). diff --git a/scripts/output/bool.review.md b/scripts/output/bool.review.md new file mode 100644 index 000000000..1cfc418ed --- /dev/null +++ b/scripts/output/bool.review.md @@ -0,0 +1,18 @@ +# Review: moonbitlang/core/bool + +**File:** `bool/pkg.generated.mbti` +**Date:** 2025-10-09T09:56:31.658Z +**Status:** ✓ Success + +--- + +**Assessment** +- API stays minimal: Bool exposes only numeric conversion helpers plus `Hash`, so surface is easy to understand and keeps implementation choices private. + +**Issues / Oddities** +- Conversion set feels uneven: we have `to_int`, `to_int16`, `to_int64` but no explicit `to_int8`/`to_int32`; similar gaps on the unsigned side (`to_uint8`/`to_uint32`). +- Method naming mixes generic and bit-width forms (`to_uint` vs `to_uint16`/`to_uint64`), which could hint at inconsistent semantics if `UInt` is not simply an alias for one of the fixed-width types. + +**Suggestions** +- Consider adding the missing sized conversions (8/32-bit) or clarifying why they are excluded; otherwise remove the 16-bit variants to keep the API tight. +- Align naming: if `UInt` is the canonical machine word, ensure docs (or naming) make the relationship to the fixed-width versions obvious. diff --git a/scripts/output/buffer.review.md b/scripts/output/buffer.review.md new file mode 100644 index 000000000..4e835c09f --- /dev/null +++ b/scripts/output/buffer.review.md @@ -0,0 +1,19 @@ +# Review: moonbitlang/core/buffer + +**File:** `buffer/pkg.generated.mbti` +**Date:** 2025-10-09T09:56:50.236Z +**Status:** ✓ Success + +--- + +**Findings** +- `buffer/pkg.generated.mbti:42` exposes `#alias(write_stringview, deprecated)` without an accompanying symbol; it is unclear what it aliases, which can confuse consumers and tooling that expect a concrete target. +- `buffer/pkg.generated.mbti:55` exports `pub typealias Buffer as T`, but the single-letter alias adds little value while increasing collision risk with local type parameters named `T`. + +**Assessment** +- Constructors (`from_array`, `from_bytes`, `from_iter`, `new`) give good coverage for common sources, and the write API is consistent across byte, numeric, and UTF encodings. +- `write_object(Self, &Show)` complements the rich set of primitives, ensuring any `Show` implementer can be serialized. + +**Suggestions** +1. Replace the deprecated alias with either a clearly documented wrapper function or remove it if the old entry point is no longer supported. +2. Drop or rename the `T` alias unless there is a strong compatibility requirement; consider a more descriptive name if an alias must remain. diff --git a/scripts/output/builtin.review.md b/scripts/output/builtin.review.md new file mode 100644 index 000000000..8cf5b7b19 --- /dev/null +++ b/scripts/output/builtin.review.md @@ -0,0 +1,18 @@ +# Review: moonbitlang/core/builtin + +**File:** `builtin/pkg.generated.mbti` +**Date:** 2025-10-09T09:59:18.924Z +**Status:** ✓ Success + +--- + +**Assessment** +- Interface offers comprehensive core facilities (errors, collections, numerics, traits) and keeps deprecation markers visible, giving consumers a clear view of the stable vs. legacy surface. + +**Issues** +- `ArgsLoc::to_json` returns `String` rather than `Json`, which breaks the convention established by every other `*_::to_json` helper and risks confusion with the `ToJson` trait (`builtin/pkg.generated.mbti:68`). +- `Default` trait impls cover `Bool`, integral types, `Double`, and `FixedArray`, but skip peers such as `Float` and `UInt`, creating an inconsistent numeric story (contrast `builtin/pkg.generated.mbti:819-826`). + +**Suggestions** +- Adjust `ArgsLoc::to_json` to return `Json` (or rename it to clarify it produces a textual representation) so the helper aligns with the rest of the package. +- Either add `Default` impls for `Float`/`UInt` (and other obvious types) or document why they are intentionally missing to avoid surprises for users expecting parity with `Double` and signed integers. diff --git a/scripts/output/byte.review.md b/scripts/output/byte.review.md new file mode 100644 index 000000000..d159c0ec0 --- /dev/null +++ b/scripts/output/byte.review.md @@ -0,0 +1,13 @@ +# Review: moonbitlang/core/byte + +**File:** `byte/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:06.809Z +**Status:** ✓ Success + +--- + +**API Review** +- `byte/pkg.generated.mbti` Minimal surface feels focused: Byte exposes bounds plus two utility methods, keeping the API easy to learn. +- `byte/pkg.generated.mbti:4` Using package-level `max_value`/`min_value` instead of associated constants can make the API feel unlike other numeric types; if the ecosystem favors `Byte::MAX`-style naming, aligning would reduce surprise. +- `byte/pkg.generated.mbti:8` `Byte::popcnt` returns `Int`, which adds an unnecessary sign and width change for a result in 0–8; consider `UInt8` (or at least `UInt`) to avoid extra casts downstream. +- `byte/pkg.generated.mbti:9` Lone `to_uint64` suggests an incomplete conversion story; either clarify (e.g., rename to `as_uint64`) or round out conversions/traits so callers don’t wonder why only 64-bit is supported. diff --git a/scripts/output/bytes.review.example.md b/scripts/output/bytes.review.example.md new file mode 100644 index 000000000..0ea4d249b --- /dev/null +++ b/scripts/output/bytes.review.example.md @@ -0,0 +1,58 @@ +# Review: moonbitlang/core/bytes + +**File:** `bytes/pkg.generated.mbti` +**Date:** 2025-10-09T12:34:56.789Z +**Status:** ✓ Success + +--- + +## API Design Assessment + +The Bytes package has a well-designed API with clear separation between immutable `Bytes` and view-based `BytesView` types. The interface provides comprehensive functionality for byte manipulation. + +### Strengths: + +1. **Clear Type Distinction**: The separation between `Bytes` (immutable) and `BytesView` (efficient slicing) is excellent for both safety and performance. + +2. **Comprehensive Conversion**: Good coverage of conversions: + - Array/FixedArray interop + - Iterator support + - String views + +3. **New Addition**: The `repeat` function is a valuable addition that fills a common use case. + +4. **Consistent Naming**: Function names follow clear conventions (`from_*`, `to_*`, `unsafe_*`). + +### Potential Issues: + +1. **Unsafe Operations**: Multiple `unsafe_extract_*` functions are exposed. Consider: + - Adding safe alternatives with bounds checking + - Clear documentation about when unsafe operations are appropriate + - Consider marking them with `#internal` if not meant for general use + +2. **BytesView Type Alias**: The `pub typealias BytesView as View` might cause confusion. Consider: + - Using the full name `BytesView` consistently + - Or deprecating one in favor of the other + +3. **Missing Operations**: Consider adding: + - `concat` or `join` for combining multiple bytes + - `split` for breaking bytes at delimiters + - `trim` operations for removing leading/trailing bytes + +### Suggestions: + +1. **Documentation**: Add examples for the `unsafe_*` functions showing proper usage and bounds requirements. + +2. **Safety Wrappers**: Consider providing safe alternatives: + ```moonbit + fn extract_bit(self: Bytes, offset: Int, length: Int) -> UInt? + fn extract_byte(self: Bytes, offset: Int, length: Int) -> UInt? + ``` + +3. **Consistency**: The `repeat` function is great - consider similar additions like `pad`, `truncate`, etc. + +## Overall + +Strong API design with good type safety and comprehensive functionality. The main improvement area is around unsafe operations - either provide safe alternatives or improve documentation about their proper use. + +**Rating:** 8.5/10 diff --git a/scripts/output/bytes.review.md b/scripts/output/bytes.review.md new file mode 100644 index 000000000..1a804a111 --- /dev/null +++ b/scripts/output/bytes.review.md @@ -0,0 +1,22 @@ +# Review: moonbitlang/core/bytes + +**File:** `bytes/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:14.339Z +**Status:** ✓ Success + +--- + +**API Assessment** +- Surface gives core builders (`from_*`, `to_*`) plus search, slicing, iteration—covers common byte needs. +- Clear split between safe (`get`, `sub`) and unchecked (`unsafe_extract_*`) primitives, with aliases for subscripting. +- BytesView includes numeric conversions, hashing, comparison traits—useful for read-only parsing. + +**Potential Issues** +- `impl Hash for Bytes` without a matching `Eq`/`Compare` contract (`bytes/pkg.generated.mbti`) breaks the usual expectation that hashable types are equality-comparable. +- `Bytes` lacks a direct `length`/`len` accessor while `BytesView` has `length`; forces consumers to convert to a view or iterate just to get size. +- Redundant constructors: `Bytes::from_fixedarray` (with optional `len`) and `Bytes::of` both wrap `FixedArray[Byte]`, which can confuse users about recommended usage. + +**Suggestions** +- Add `impl Eq` (and `Compare` if ordering is meaningful) for `Bytes`, or drop `Hash` until equality semantics are defined. +- Expose a `Bytes::length` (or `Bytes::len`) to align with `BytesView::length` and avoid needless conversions. +- Consolidate `Bytes::from_fixedarray`/`Bytes::of` into a single, clearly named constructor—perhaps keep one public function and deprecate the other for clarity. diff --git a/scripts/output/char.review.md b/scripts/output/char.review.md new file mode 100644 index 000000000..f9d7dc546 --- /dev/null +++ b/scripts/output/char.review.md @@ -0,0 +1,17 @@ +# Review: moonbitlang/core/char + +**File:** `char/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:23.686Z +**Status:** ✓ Success + +--- + +**API Review** +- The surface mirrors familiar `Char` classification helpers (ASCII filters, Unicode-aware checks, hash/show/json impls), so discoverability should be good for users coming from Rust-like ecosystems. + +**Findings** +- char/pkg.generated.mbti: `Char::utf16_len` returns `Int`, even though the result is logically bounded to 1 or 2; using a signed type is slightly inconsistent with the rest of the API and invites negative values in user code. + +**Suggestions** +- 1. Consider switching `utf16_len` to return an unsigned width (`UInt`) or a dedicated enum to emphasize the constrained range. +- 2. If broader Unicode casing is supported elsewhere, exposing `to_lowercase`/`to_uppercase` counterparts (even if they return sequences) would round out the API; otherwise document that only ASCII case conversions are offered. diff --git a/scripts/output/cmp.review.md b/scripts/output/cmp.review.md new file mode 100644 index 000000000..cf0f38511 --- /dev/null +++ b/scripts/output/cmp.review.md @@ -0,0 +1,23 @@ +# Review: moonbitlang/core/cmp + +**File:** `cmp/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:19.673Z +**Status:** ✓ Success + +--- + +**API Notes** + +- Public functions cover both direct comparisons and key-based comparisons for min/max, which is a clean surface; tuple result from `minmax` variants is intuitive, but the ordering (min first vs max first) should be documented somewhere user-facing to avoid ambiguity. +- Key-based overloads require both operands plus a key function; consider clarifying tie-breaking behavior when keys compare equal. + +**Issues / Inconsistencies** + +- `Reverse::inner` remains exposed (though deprecated); having a deprecated accessor without pointing to the preferred replacement can confuse users relying on the generated interface alone. +- `Reverse[T]` wraps a value but gives no hint about its construction or intended usage in the public surface—if users are meant to construct it manually, a constructor (even implicit) should be evident, or otherwise mark the struct as opaque. + +**Suggestions** + +1. Add doc comments (or complementary docs) clarifying tuple ordering and tie resolution for the `minmax*` functions. +2. Either remove the deprecated `Reverse::inner` from the interface once callers have migrated, or document the alternative explicitly; if the type should remain transparent, provide a non-deprecated accessor. +3. Consider exposing an obvious constructor/helper for `Reverse[T]` so users don’t rely on struct internals, or make the struct opaque if manual construction is discouraged. diff --git a/scripts/output/coverage.review.md b/scripts/output/coverage.review.md new file mode 100644 index 000000000..fa60e2772 --- /dev/null +++ b/scripts/output/coverage.review.md @@ -0,0 +1,23 @@ +# Review: moonbitlang/core/coverage + +**File:** `coverage/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:27.939Z +**Status:** ✓ Success + +--- + +**Assessment** +- Extremely small surface: global `end()` and `track()` plus `CoverageCounter` with `new`/`incr` and `Show`. Focused but opaque—callers get no insight into lifecycle or state beyond printing. + +**Potential Issues** +- `CoverageCounter::incr(Self, Int)` consumes `Self`; if `Self` isn’t a lightweight handle, increments may act on a copy and lose state. +- `track(String, CoverageCounter) -> Unit` offers no return value or feedback; unclear whether it clones, mutates, or owns the counter. +- `end()` with no parameters/return value is ambiguous—unclear which counters or session it affects. +- No accessor to read a counter’s value; callers must rely on `Show`, which is unusual for programmatic use. +- Lack of docs makes sequencing (must `track` follow `new`? when to `end`?) and ownership rules unclear. + +**Suggestions** +- Clarify parameter passing and mutability: accept counters by reference or provide docs guaranteeing value semantics. +- Consider returning status/data from `track` or exposing a `value()` accessor on `CoverageCounter`. +- Rename or document `end()` to make its scope explicit (e.g., `finalize_session()`). +- Provide minimal usage guidance in docs or through API naming to communicate lifecycle expectations. diff --git a/scripts/output/deque.review.md b/scripts/output/deque.review.md new file mode 100644 index 000000000..9ced3ae67 --- /dev/null +++ b/scripts/output/deque.review.md @@ -0,0 +1,17 @@ +# Review: moonbitlang/core/deque + +**File:** `deque/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:39.922Z +**Status:** ✓ Success + +--- + +**Findings** +- `deque/pkg.generated.mbti:14` – `Deque::append(Self[A], Self[A]) -> Unit` takes two `Self` parameters and returns `Unit`, so it is unclear which deque is mutated. A signature like `(Self[A], Self[A]) -> Self[A]` or naming the receiver explicitly would make the mutation semantics obvious. +- `deque/pkg.generated.mbti:20` – `Deque::binary_search_by(Self[A], (A) -> Int)` uses a predicate returning `Int`. Most comparison callbacks in the ecosystem return an `Ordering`/`Compare` result; using raw `Int` encourages ad‑hoc conventions and risks misuse. +- `deque/pkg.generated.mbti:51` & `deque/pkg.generated.mbti:52` – `unsafe_pop_back` and `unsafe_pop_front` return `Unit`, unlike the safe `pop_*` variants that return the removed element. That forces callers to pair them with an extra lookup, defeating the point of an “unsafe but fast” API. + +**Suggestions** +- Clarify the receiver on mutating helpers like `append`, or return the mutated deque to mirror other builders. +- Change `binary_search_by`’s callback to return the standard ordering type (or document the expected `Int` contract) to keep the API predictable. +- Consider having the `unsafe_pop_*` methods return the removed element; otherwise rename them to emphasize they only drop elements so users don’t assume they yield a value. diff --git a/scripts/output/double.review.md b/scripts/output/double.review.md new file mode 100644 index 000000000..0bef920fe --- /dev/null +++ b/scripts/output/double.review.md @@ -0,0 +1,21 @@ +# Review: moonbitlang/core/double + +**File:** `double/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:51.824Z +**Status:** ✓ Success + +--- + +**API Notes** +- Balanced coverage of constants and predicates: classification helpers (`is_nan`, `is_inf`, `signum`) and rounding utilities look coherent with the rest of the numeric core (`double/pkg.generated.mbti:21`–`double/pkg.generated.mbti:51`). +- Having both a package-level `from_int` and an associated `Double::from_int` is consistent with other MoonBit modules but feels redundant unless both entry points serve distinct ergonomics (`double/pkg.generated.mbti:5`, `double/pkg.generated.mbti:28`). + +**Possible Issues** +- `Double::inf(Int)` remains exposed even while deprecated; the `Int` parameter is confusing alongside the constant `infinity`, and keeping it in the primary interface instead of a dedicated deprecated surface increases the chance of accidental use (`double/pkg.generated.mbti:29`–`double/pkg.generated.mbti:31`, `double/pkg.generated.mbti:7`). +- `Double::to_uint` is the sole lossy conversion back to integers; without `to_int` or a fallible variant, it is unclear how negative values or large magnitudes are handled, which could surprise API users (`double/pkg.generated.mbti:49`). +- Optional arguments on `Double::is_close` lack documented defaults in the interface, so callers cannot infer what happens when they omit tolerances (`double/pkg.generated.mbti:32`). + +**Suggestions** +1. Either move deprecated members such as `Double::inf`, `Double::min_normal`, and `Double::nan` into a dedicated `deprecated.mbt` or document clearly why they must stay in the main interface. +2. Clarify or complement `Double::to_uint` with a documented contract (clamping, truncation, errors) and consider adding a signed counterpart for symmetry. +3. Document the default tolerances for `Double::is_close` (or surface overloads without optionals) so users know the comparison policy without digging into the implementation. diff --git a/scripts/output/double_internal_ryu.review.md b/scripts/output/double_internal_ryu.review.md new file mode 100644 index 000000000..b1353069b --- /dev/null +++ b/scripts/output/double_internal_ryu.review.md @@ -0,0 +1,11 @@ +# Review: moonbitlang/core/double/internal/ryu + +**File:** `double/internal/ryu/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:35.637Z +**Status:** ✓ Success + +--- + +- `double/internal/ryu/pkg.generated.mbti:4` Exposed surface is a single helper `ryu_to_string(Double) -> String`, which keeps the internal Ryu encoder narrowly scoped—sensible for an internal package. +- `double/internal/ryu/pkg.generated.mbti:4` Potential gap: no surface-level clues about formatting specifics (rounding mode, NaN/inf handling, locale). Callers must infer behavior from implementation, which can lead to misuse. +- Suggest documenting the exact formatting guarantees (IEEE 754 compliance, shortest-roundtrip, special-value output) in the parent package or via comments in the interface generator, so downstream internal callers understand when to prefer this over other conversions. diff --git a/scripts/output/encoding_utf8.review.md b/scripts/output/encoding_utf8.review.md new file mode 100644 index 000000000..d2c3bd407 --- /dev/null +++ b/scripts/output/encoding_utf8.review.md @@ -0,0 +1,19 @@ +# Review: moonbitlang/core/encoding/utf8 + +**File:** `encoding/utf8/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:43.757Z +**Status:** ✓ Success + +--- + +**API Review** +- decode/decode_lossy/encode cover the minimal UTF-8 roundtrip; explicit `ignore_bom?`/`bom?` flags give callers control over BOM handling while keeping defaults straightforward. +- Exposing a dedicated `Malformed BytesView` error type is clear and keeps malformed input diagnostics specific to UTF-8. + +**Potential Issues** +- `decode` raises `Malformed` but the signature does not indicate what part of decoding failed—consider surfacing offset information or clarifying error semantics in docs (`encoding/utf8/pkg.generated.mbti`). +- `decode_lossy` silently drops errors; if it substitutes replacement characters, that policy should be documented or surfaced via an enum/strategy parameter. + +**Suggestions** +- Document default behavior of the optional BOM flags and whether BOMs are stripped by default. +- Evaluate whether `Malformed` should expose more context (e.g., byte index) or whether additional helper APIs (peek BOM, validate only) would improve usability. diff --git a/scripts/output/env.review.md b/scripts/output/env.review.md new file mode 100644 index 000000000..f2175ca0a --- /dev/null +++ b/scripts/output/env.review.md @@ -0,0 +1,18 @@ +# Review: moonbitlang/core/env + +**File:** `env/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:54.830Z +**Status:** ✓ Success + +--- + +**Findings** +- Major — `env/pkg.generated.mbti:9` — `now()` exposes raw `UInt64` without documenting epoch/units; callers must infer semantics and risk misinterpreting wall-clock vs monotonic time. +- Minor — `env/pkg.generated.mbti:7` — `current_dir()` returns `String?`, but no hint of failure modes or how to detect/diagnose errors; a `Result[String, Error]` (or documented `None` cases) would be clearer. + +**Assessment** +- Very small surface; names are intuitive, but the lack of type safety or documentation around environment-dependent behavior makes the API easy to misuse. + +**Suggestions** +- Document observable behavior (arg ordering, inclusion of program name, guarantees for absence of CWD, timestamp origin). +- Prefer stronger typing: introduce a `Timestamp` alias/struct (with epoch/unit info) and consider `Result`-style errors instead of bare optionals where diagnostics matter. diff --git a/scripts/output/error.review.md b/scripts/output/error.review.md new file mode 100644 index 000000000..b4264eac4 --- /dev/null +++ b/scripts/output/error.review.md @@ -0,0 +1,11 @@ +# Review: moonbitlang/core/error + +**File:** `error/pkg.generated.mbti` +**Date:** 2025-10-09T09:57:52.750Z +**Status:** ✓ Success + +--- + +- **API Outlook**: Interface currently exports only trait impls (`Show`, `ToJson`) for `Error`; no values, constructors, or documented type definition appear, so practical surface for consumers is opaque. +- **Potential Issues**: Missing `Error` declaration in the interface suggests the type might not be publicly constructible or inspectable; if the type is meant to be used externally, callers can see it but cannot create or pattern-match it. Lack of exposed helpers leaves trait impls largely unusable. +- **Suggestions**: 1) Expose the `Error` type definition or at least public constructors/wrappers so clients can produce errors. 2) Consider adding factory functions or constants for common error cases to make the package useful. 3) Document intended usage of `Error` so trait impls have clear context. diff --git a/scripts/output/float.review.md b/scripts/output/float.review.md new file mode 100644 index 000000000..1aef625ee --- /dev/null +++ b/scripts/output/float.review.md @@ -0,0 +1,20 @@ +# Review: moonbitlang/core/float + +**File:** `float/pkg.generated.mbti` +**Date:** 2025-10-09T09:58:03.763Z +**Status:** ✓ Success + +--- + +**API Notes** +- `float/pkg.generated.mbti` exposes the expected numeric utilities: zero-ish `default`, IEEE‑754 constants, and standard math helpers (`abs`, `ceil`, `floor`, `round`, `trunc`, `pow`), plus hashing/show impls. +- Optional parameters on `Float::is_close` let callers fine‑tune tolerance without overloading. + +**Potential Issues** +- `Float::to_int` (`float/pkg.generated.mbti`) returns an `Int` without signalling what happens for NaN, infinities, or values outside the integer range—risk of silent UB or truncation surprises. +- `is_close` lists `relative_tolerance` before `absolute_tolerance`; most libraries favor the opposite order, so callers may guess wrong when using named/positional args. + +**Suggestions** +1. Clarify or split `to_int` (e.g., document clamping/rounding rules or provide checked variants). +2. Consider reordering `is_close` parameters (absolute first) or requiring named arguments to reduce call-site ambiguity. +3. If feasible, add symmetric constructors (`from_be_bytes`, etc.) to complement the byte-conversion helpers. diff --git a/scripts/output/hashmap.review.md b/scripts/output/hashmap.review.md new file mode 100644 index 000000000..84a97e249 --- /dev/null +++ b/scripts/output/hashmap.review.md @@ -0,0 +1,24 @@ +# Review: moonbitlang/core/hashmap + +**File:** `hashmap/pkg.generated.mbti` +**Date:** 2025-10-09T09:58:23.237Z +**Status:** ✓ Success + +--- + +**API Assessment** +- Coverage is solid: construction (`new`, `from_array`, `from_iter`, `of`), lookup variants (`at`, `get`, fallbacks), mutation (`set`, `remove`, `retain`, `clear`), traversal (`iter`, `iter2`, `each`, `eachi`, `keys`, `values`), conversion (`to_array`, `map`, `copy`). +- Trait coverage (`Default`, `Eq`, `Show`, `ToJson`, `quickcheck::Arbitrary`) makes the type easy to integrate and test. +- Operator aliases (`_[ _ ]`, `_[_]=_`) align with familiar map ergonomics. + +**Potential Issues** +- `HashMap::remove` returns `Unit`, so callers can’t tell if a key existed; most map APIs surface a bool or removed value for clarity. +- `HashMap::map` is value-only; the name may suggest key/value transformation—even though keys stay fixed, which could mislead users expecting a general map over `(K, V)`. +- `HashMap::at` (alias `_[_]`) returns bare `V` and likely panics on missing keys. That’s standard for indexing, but the documentation should make the panic contract explicit. +- Alias `size` is marked deprecated, but users only see that via the attribute; consider reinforcing this in docs or migration notes. +- `copy` implies full duplication; without context on cost or mutability semantics, users might misuse it for cheap clones. + +**Suggestions** +1. Consider returning `Bool` or `V?` from `remove` (or adding `remove_entry`) to signal success. +2. Rename `map` to `map_values` or document prominently that keys are untouched; alternatively add a separate value mapper to avoid overloaded expectations. +3. Document panic behavior for `at`/`_[ _ ]` and the intended replacement for deprecated `size`. diff --git a/scripts/output/hashset.review.md b/scripts/output/hashset.review.md new file mode 100644 index 000000000..ca9a679ef --- /dev/null +++ b/scripts/output/hashset.review.md @@ -0,0 +1,20 @@ +# Review: moonbitlang/core/hashset + +**File:** `hashset/pkg.generated.mbti` +**Date:** 2025-10-09T09:58:22.424Z +**Status:** ✓ Success + +--- + +**API Review** +- HashSet exposes the expected set operations (insert/remove/membership plus union/diff/intersection) and standard iteration helpers. +- Constructors cover several common sources (empty with optional capacity, array, iter, fixed array), giving flexible creation paths. +- Trait implementations (BitAnd/BitOr/BitXOr/Sub/Default/Show/ToJson/Arbitrary) make the type integrate well with the broader ecosystem. + +**Issues & Inconsistencies** +- `HashSet::length` is marked `#alias(size, deprecated)` but the primary `size` method does not appear in this interface (`hashset/pkg.generated.mbti:23`); users may wonder what to call instead. +- `HashSet::copy` has no `Hash + Eq` (or other) constraint on `K` (`hashset/pkg.generated.mbti:10`), yet cloning every element typically requires at least `Clone`/`Copy`— worth confirming the semantics. + +**Suggestions** +1. Expose the intended replacement for `length` (`size`?) in the public interface so users can migrate cleanly. +2. Clarify (in docs or signature bounds) how `HashSet::copy` duplicates elements, or add the appropriate trait requirements if element copies are deep. diff --git a/scripts/output/immut_array.review.md b/scripts/output/immut_array.review.md new file mode 100644 index 000000000..3ffd68ed3 --- /dev/null +++ b/scripts/output/immut_array.review.md @@ -0,0 +1,22 @@ +# Review: moonbitlang/core/immut/array + +**File:** `immut/array/pkg.generated.mbti` +**Date:** 2025-10-09T09:58:08.962Z +**Status:** ✓ Success + +--- + +**API Assessment** +- Comprehensive coverage for immutable array basics: creation (`new`, `make`, `from_array`, `from_iter`), access (`at`, `get`, `iter`), transformation (`map`, `fold`, `rev_fold`), and growth (`push`, `concat`). +- Trait coverage (`Eq`, `Compare`, `Hash`, `Show`, `Add`, `@quickcheck.Arbitrary`) makes the type integrate well across the ecosystem. +- Optionality of `at` vs `get` offers both strict and safe access patterns. + +**Potential Issues** +- `at` and `set` signatures imply they raise on out-of-bounds, but the interface doesn’t clarify whether they trap, raise, or silently handle errors. +- `push`/`concat` on immutable arrays can be expensive; nothing hints at their complexity, which may surprise users. +- Deprecated entries (`copy`, `fold_left`, `fold_right`) remain exported; consumers may still rely on them without seeing alternatives. + +**Suggestions** +- Document failure modes (or switch to returning `A?` / `Self[A]?`) for bounds-sensitive operations to make the API safer. +- Consider adding lightweight builders or batched operations to offset the cost of repeated `push`/`concat`. +- If deprecations stay visible, add explicit replacements in docs, or move them to a `deprecated.mbt` block to keep the primary surface clean. diff --git a/scripts/output/immut_hashmap.review.md b/scripts/output/immut_hashmap.review.md new file mode 100644 index 000000000..a90e4e27f --- /dev/null +++ b/scripts/output/immut_hashmap.review.md @@ -0,0 +1,18 @@ +# Review: moonbitlang/core/immut/hashmap + +**File:** `immut/hashmap/pkg.generated.mbti` +**Date:** 2025-10-09T09:59:07.082Z +**Status:** ✓ Success + +--- + +**Findings** +- `HashMap::difference`, `HashMap::intersection`, `HashMap::intersection_with`, `HashMap::union`, and `HashMap::union_with` only constrain `K : Eq` (`immut/hashmap/pkg.generated.mbti:18`, `immut/hashmap/pkg.generated.mbti:32`, `immut/hashmap/pkg.generated.mbti:33`, `immut/hashmap/pkg.generated.mbti:51`, `immut/hashmap/pkg.generated.mbti:52`). These set operations need to hash keys from both operands; without `Hash`, callers can construct `HashMap`s (e.g., via `new`) whose keys are not hashable yet still invoke these functions, leading to either compile gaps in downstream code or subtle runtime assumptions. +- `HashMap::singleton` requires `K : Hash` but omits `Eq` (`immut/hashmap/pkg.generated.mbti:49`). Any collision handling still needs equality tests, and most other constructors (e.g., `add`, `from_array`) require both traits, so this asymmetry is surprising and risks misuse. + +**API Assessment** +- Broadly complete immutable dictionary surface: bulk constructors (`from_iter`, `of`), structural updates (`add`, `remove`), rich iteration (`iter`, `iter2`, `keys`, `values`), and algebraic combinators (`union`, `intersection`, etc.). Deprecated aliases remain for legacy callers, and free-function aliases ease import ergonomics. + +**Suggestions** +- Tighten trait bounds so every operation that inspects hashes requires `Hash` (and `Eq` where comparisons occur) for consistency and clearer expectations. In practice that means adding `Hash` to the set-operations above and adding `Eq` to `singleton`. +- Consider documenting or deprecating remaining legacy methods (`filter`, `map`, `fold`) in favor of the `*_with_key` variants in the main docs so users know which surface is preferred. diff --git a/scripts/output/immut_hashset.review.md b/scripts/output/immut_hashset.review.md new file mode 100644 index 000000000..2d79d92b7 --- /dev/null +++ b/scripts/output/immut_hashset.review.md @@ -0,0 +1,15 @@ +# Review: moonbitlang/core/immut/hashset + +**File:** `immut/hashset/pkg.generated.mbti` +**Date:** 2025-10-09T09:58:42.840Z +**Status:** ✓ Success + +--- + +Solid functional set API: covers creation (`new`, `of`, `from_*`), membership (`contains`, `is_empty`, `length`), iteration (`iter`, `each`), and set algebra (`union`, `intersection`, `difference`) with the expected typeclass instances. + +Potential issues: +- `HashSet::difference`, `HashSet::intersection`, and `HashSet::union` constrain their element type with `Eq` only, while every constructor/update already needs both `Eq + Hash`. Building the result of these operations inevitably hashes the elements, so callers with an element type that lacks `Hash` would get a compile-time failure inside the implementation rather than at the call site. That’s inconsistent with the rest of the interface and makes the API surface misleading. + +Suggestions: +- Tighten the bounds on `difference`, `intersection`, and `union` to `K : Eq + Hash` so their signatures reflect the actual requirements and stay consistent with the rest of the set API. diff --git a/scripts/output/immut_internal_path.review.md b/scripts/output/immut_internal_path.review.md new file mode 100644 index 000000000..286dea4a6 --- /dev/null +++ b/scripts/output/immut_internal_path.review.md @@ -0,0 +1,22 @@ +# Review: moonbitlang/core/immut/internal/path + +**File:** `immut/internal/path/pkg.generated.mbti` +**Date:** 2025-10-09T09:58:43.201Z +**Status:** ✓ Success + +--- + +**Findings** +- immut/internal/path/pkg.generated.mbti:11 – `Path::idx` exposes the index as `Int` even though the underlying storage is `UInt`; the implicit narrowing risks overflow/negative values and makes the API inconsistent. +- immut/internal/path/pkg.generated.mbti:16 – `Path::push` accepts `Int` while the path state is `UInt`; the mismatch invites accidental negative inputs and suggests callers convert manually. +- immut/internal/path/pkg.generated.mbti:12 – `Path::inner` remains publicly accessible despite being deprecated, but there is no visible replacement, so external callers may keep relying on it. + +**Assessment** +- API surface is very small and focused, but several signatures expose inconsistent integer types and the deprecation lacks a clear migration path. + +**Open Questions** +- Should `Path` indexes always be non-negative/fit in `UInt`, or is `Int` truly required for external interoperability? + +**Suggestions** +1. Align index-related parameters/returns (`idx`, `push`) with `UInt` (or introduce explicit conversion helpers) so callers don’t juggle signed/unsigned mismatches. +2. Provide a non-deprecated accessor or constructor that replaces `inner`, or fully gate it behind internal visibility to discourage continued external use. diff --git a/scripts/output/immut_internal_sparse_array.review.md b/scripts/output/immut_internal_sparse_array.review.md new file mode 100644 index 000000000..46e094e0b --- /dev/null +++ b/scripts/output/immut_internal_sparse_array.review.md @@ -0,0 +1,19 @@ +# Review: moonbitlang/core/immut/internal/sparse_array + +**File:** `immut/internal/sparse_array/pkg.generated.mbti` +**Date:** 2025-10-09T09:59:01.158Z +**Status:** ✓ Success + +--- + +**Assessment** +- `SparseArray` offers a solid immutable-style surface (`add`, `remove`, `replace`, set-like ops) and mirrors `Bitset` helpers, so the core use cases appear covered (`immut/internal/sparse_array/pkg.generated.mbti:5-28`). +- Callback-based combinators (`map`, `each`, `filter`, `union`, etc.) support effectful code via `raise?`, which keeps the API flexible without exposing internals (`immut/internal/sparse_array/pkg.generated.mbti:21-28`). + +**Issues** +- `SparseArray::filter` and `SparseArray::difference` both return `Self[X]?`, yet an empty sparse array is already representable via `empty()`. The extra option layer suggests an error state that is not documented or justified, making the signature confusing (`immut/internal/sparse_array/pkg.generated.mbti:19-22`). +- `Bitset::inner` is deprecated, but there is no non-deprecated alternative for extracting raw contents or iterating all set indices, nudging callers back to a deprecated API (`immut/internal/sparse_array/pkg.generated.mbti:12-17`). + +**Suggestions** +1. Clarify the meaning of the `None` result (or drop it) for `SparseArray::filter`/`difference`; if it only signals “no surviving elements”, return a plain `SparseArray[X]` with `empty()` instead. +2. Provide a supported replacement for `Bitset::inner`, e.g., a safe iterator or an explicit constructor/extractor, so users can leave the deprecated path without losing functionality. diff --git a/scripts/output/immut_list.review.md b/scripts/output/immut_list.review.md new file mode 100644 index 000000000..e24fbde6b --- /dev/null +++ b/scripts/output/immut_list.review.md @@ -0,0 +1,23 @@ +# Review: moonbitlang/core/immut/list + +**File:** `immut/list/pkg.generated.mbti` +**Date:** 2025-10-09T09:58:59.436Z +**Status:** ✓ Success + +--- + +Most of the exposed surface in `immut/list/pkg.generated.mbti:1` is marked `#deprecated`, including every constructor, constructor-like helper, and nearly all instance methods, yet they remain publicly exported. That sends a confusing signal to downstream users because there is no obvious non-deprecated replacement. + +Potential issues or inconsistencies: +- `default`/`from_array`/`from_iter`/`from_json` appear twice (as free functions and as `T::…` methods), all deprecated; it isn’t clear which form (if any) should still be used. +- `T::to_json`/`T::from_json` stay deprecated while the type still implements `ToJson`/`@json.FromJson`, so users get mixed messages about serialization support. +- Keeping the entire legacy surface exported makes it hard to rely on the enum alone without accidentally touching deprecated APIs. + +Suggestions: +1. Either undeprecate the constructors and core list operations or split them into a legacy module so the default interface isn’t wall-to-wall deprecations. +2. If the goal is to replace this API, surface the intended successors (e.g., new module or iterators) and document migration guidance; otherwise prune the deprecated entries from the generated interface. +3. Consider collapsing the redundant free-function vs. method duplicates to a single canonical shape once the deprecation story is settled. + +Natural next steps, if you want to pursue them: +1. Decide which functions to keep and move any legacy helpers into `deprecated.mbt`. +2. Regenerate the interface with `moon info` once the public surface is clarified. diff --git a/scripts/output/immut_priority_queue.review.md b/scripts/output/immut_priority_queue.review.md new file mode 100644 index 000000000..b26fe4e12 --- /dev/null +++ b/scripts/output/immut_priority_queue.review.md @@ -0,0 +1,12 @@ +# Review: moonbitlang/core/immut/priority_queue + +**File:** `immut/priority_queue/pkg.generated.mbti` +**Date:** 2025-10-09T09:59:05.818Z +**Status:** ✓ Success + +--- + +- In `immut/priority_queue/pkg.generated.mbti` the safe removal path splits responsibilities between `peek` (returns `A?`) and `pop` (returns `Self[A]?`). This forces callers to make two calls to obtain and remove the head element, which is awkward for a data structure that conceptually returns the removed element. Consider exposing a combined API (e.g. returning `(A, Self[A])?`) or clarifying/documenting the intended usage. +- Both `pop_exn` (deprecated) and `unsafe_pop` remove the head without returning it. Keeping two APIs with similar, failure-prone semantics is confusing; either retire `pop_exn` fully or ensure there is a clear behavioral difference. +- Several read-only operations (`iter`, `to_array`) require `Compare`, even though iteration or copying out likely doesn’t need to recompare elements. Relaxing these bounds would make the API friendlier for queues of types that only need `Compare` at construction time. +- Overall the surface is otherwise coherent: constructors cover arrays/iters, mutation returns new queues (consistent with immutability), and trait impls (`Compare`, `Eq`, `Hash`, `Show`, `ToJson`, QuickCheck) align with expectations. diff --git a/scripts/output/immut_sorted_map.review.md b/scripts/output/immut_sorted_map.review.md new file mode 100644 index 000000000..777ebca38 --- /dev/null +++ b/scripts/output/immut_sorted_map.review.md @@ -0,0 +1,20 @@ +# Review: moonbitlang/core/immut/sorted_map + +**File:** `immut/sorted_map/pkg.generated.mbti` +**Date:** 2025-10-09T09:59:49.292Z +**Status:** ✓ Success + +--- + +**API Review** +- Comprehensive surface for construction, querying, and traversal; aliases for deprecated names ease migration (`immut/sorted_map/pkg.generated.mbti:15-71`). + +**Issues** +- `SortedMap::contains` lacks a `K : Compare` bound while other lookup-style helpers such as `get` and `at` require it, which makes the signature inconsistent and may surprise users (`immut/sorted_map/pkg.generated.mbti:19`). +- `SortedMap::new` is marked as a free function but omits the `K : Compare` constraint that every other constructor-style function carries, leaving ambiguity about when a user must supply the comparator (`immut/sorted_map/pkg.generated.mbti:53`). +- A large set of deprecated members still lives in the primary interface instead of a dedicated `deprecated.mbt`, so the generated API remains noisy and harder to scan (`immut/sorted_map/pkg.generated.mbti:22-50`). + +**Suggestions** +- Add the missing `Compare` bound (or document why it is unnecessary) on `contains` to match the rest of the lookup API. +- Require `K : Compare` on `new` (or provide a parallel `unsafe_new` with rationale) so constructors present a uniform contract. +- Move deprecated definitions into a separate `deprecated.mbt` module to keep the main interface lean while still exposing them for compatibility. diff --git a/scripts/output/immut_sorted_set.review.md b/scripts/output/immut_sorted_set.review.md new file mode 100644 index 000000000..0b5bc3ae9 --- /dev/null +++ b/scripts/output/immut_sorted_set.review.md @@ -0,0 +1,20 @@ +# Review: moonbitlang/core/immut/sorted_set + +**File:** `immut/sorted_set/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:03.505Z +**Status:** ✓ Success + +--- + +**API Assessment** +- Comprehensive immutable-set surface in `immut/sorted_set/pkg.generated.mbti` with standard constructors, set algebra, traversal, and JSON/QuickCheck support; consistent persistent semantics via `Self[A]` results; optional variants exist for many partial operations. + +**Issues** +- `SortedSet::max` / `SortedSet::min` (`immut/sorted_set/pkg.generated.mbti:40`, `immut/sorted_set/pkg.generated.mbti:42`) return bare `A`; calling them on an empty set will likely trap, but the signature gives no hint of that precondition. +- `SortedSet::remove_min` (`immut/sorted_set/pkg.generated.mbti:49`) both drops the removed element and provides no failure signalling, forcing callers to fetch `min` separately and handle emptiness twice. +- `SortedSet::new` (`immut/sorted_set/pkg.generated.mbti:45`) constructs an empty set without requiring `A : Compare`; it lets users form `SortedSet` values whose element type can’t satisfy core operations like `add`, which may be surprising. + +**Suggestions** +- Annotate partial operations (`min`, `max`, `remove_min`) with `raise?`, or otherwise document/enforce their preconditions so misuse is visible at compile time. +- Offer a variant of `remove_min` that returns the removed element (e.g. `(A?, Self[A])`) to avoid redundant lookups and make emptiness explicit. +- Consider tightening constructor bounds (or clearly documenting why they’re loose) so only comparable element types can inhabit `SortedSet[A]`. diff --git a/scripts/output/int.review.md b/scripts/output/int.review.md new file mode 100644 index 000000000..431e26c8c --- /dev/null +++ b/scripts/output/int.review.md @@ -0,0 +1,18 @@ +# Review: moonbitlang/core/int + +**File:** `int/pkg.generated.mbti` +**Date:** 2025-10-09T09:59:28.469Z +**Status:** ✓ Success + +--- + +**Findings** +- int/pkg.generated.mbti:13 `Int::abs` cannot represent `abs(min_value)` in a two’s-complement `Int`; the API doesn’t clarify whether it traps, saturates, or wraps, so callers can’t reason about safety. +- int/pkg.generated.mbti:14-15 `to_be_bytes` / `to_le_bytes` expose only one-way conversions and return an unsized `Bytes`; without documenting the fixed width or offering `from_*` counterparts, consumers risk endian or length mismatches. + +**Open Questions** +- What is the expected behavior when `Int::abs` receives `min_value`? (panic vs. wrap vs. return `Int`?) + +**Suggestions** +- Document or change `Int::abs` to make the `min_value` edge case explicit—e.g., return a result type or note a possible panic. +- Either document the byte-length contract of `to_{be,le}_bytes` or refactor toward a single `to_bytes(endian)` plus symmetric `from_*` helpers so callers can reliably round-trip integers. diff --git a/scripts/output/int16.review.md b/scripts/output/int16.review.md new file mode 100644 index 000000000..5c0313b78 --- /dev/null +++ b/scripts/output/int16.review.md @@ -0,0 +1,20 @@ +# Review: moonbitlang/core/int16 + +**File:** `int16/pkg.generated.mbti` +**Date:** 2025-10-09T09:59:16.076Z +**Status:** ✓ Success + +--- + +**API Notes** +- Int16 exposes the expected numeric surface: constants, magnitude helper, bit reinterpretation, and standard arithmetic/bitwise trait impls; feels consistent with other core numeric packages. +- Trait coverage looks complete for general arithmetic use, including hashing and JSON encoding. + +**Potential Issues** +- `Int16::abs` cannot represent `abs(min_value)` in two’s complement and likely returns the unchanged `min_value`; that silent edge case should be documented or handled explicitly. +- `reinterpret_as_uint16` suggests a bit-cast but the behavior (endian assumptions, preservation of negative values) isn’t spelled out; ambiguity here can lead to misuse. + +**Suggestions** +1. Document `abs`’s behavior on `min_value`, or return an option/error to avoid silent overflow. +2. Clarify that `reinterpret_as_uint16` is a bit reinterpretation (not numeric conversion) and note any constraints. +3. Consider adding a conventional widening/narrowing conversion API (`to_uint16` / `from_uint16`) if reinterpretation isn’t meant to serve that role. diff --git a/scripts/output/int64.review.md b/scripts/output/int64.review.md new file mode 100644 index 000000000..e8c786503 --- /dev/null +++ b/scripts/output/int64.review.md @@ -0,0 +1,11 @@ +# Review: moonbitlang/core/int64 + +**File:** `int64/pkg.generated.mbti` +**Date:** 2025-10-09T09:59:28.848Z +**Status:** ✓ Success + +--- + +- Solid minimal surface covering creation, limits, byte conversion, and hashing for `Int64`; naming is consistent and discoverable (`to_be_bytes`/`to_le_bytes`), and exposing `min_value`/`max_value` as constants keeps usage straightforward. +- `Int64::abs` on `int64/pkg.generated.mbti` likely overflows on `min_value`; callers get no signal whether it panics, clamps, or wraps. `Int64::from_int` and free `from_int(Int)` also omit overflow semantics—unclear whether they trap, saturate, or truncate when the source `Int` exceeds the `Int64` range. +- Consider documenting or adjusting the behavior of `abs` and the two `from_int` entry points (e.g., returning an `Option`/`Result`, or naming them `checked_…`/`wrapping_…`). If both `from_int` variants remain, explain why the free function is needed alongside the inherent method. A `from_be_bytes`/`from_le_bytes` counterpart would make the byte-conversion story symmetrical. diff --git a/scripts/output/json.review.md b/scripts/output/json.review.md new file mode 100644 index 000000000..e96aec6ac --- /dev/null +++ b/scripts/output/json.review.md @@ -0,0 +1,16 @@ +# Review: moonbitlang/core/json + +**File:** `json/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:10.979Z +**Status:** ✓ Success + +--- + +**Assessment** +- `json/pkg.generated.mbti:5-97` exposes a coherent JSON surface: parsing/validation, value interrogation helpers, and a comprehensive `FromJson` ecosystem that covers primitives, containers, and tuples up to 16 entries. + +**Potential Issues** +- None observed in the exported signatures; the generated API is internally consistent. + +**Suggestions** +- `json/pkg.generated.mbti:10` Consider offering a non-throwing `parse` helper (e.g. returning `Result[Json, ParseError]`) for consumers who prefer explicit error handling over exceptions. diff --git a/scripts/output/list.review.md b/scripts/output/list.review.md new file mode 100644 index 000000000..22a178847 --- /dev/null +++ b/scripts/output/list.review.md @@ -0,0 +1,20 @@ +# Review: moonbitlang/core/list + +**File:** `list/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:07.495Z +**Status:** ✓ Success + +--- + +**Assessment** +- Coverage is broad: predicates, folds, zips, (un)folds, and trait impls give a complete immutable list API. +- Naming mostly matches community expectations (`map`, `filter`, `fold`, etc.), and `#as_free_fn` helpers support both static and functional styles. +- Deprecations are clearly tagged, signalling safer alternatives (`tail`, `rev_fold*`, `construct`). + +**Issues** +- `List::findi` receives parameters as `(A, Int)` while every other indexed callback (`eachi`, `mapi`, `foldi`) uses `(Int, A)`; this inconsistency is surprising and error-prone (`list/pkg.generated.mbti:34`). +- `List::prepend` takes `(Self[A], A)` but is aliased as `add`, whereas `List::cons` (and the deprecated `construct`) take `(A, Self[A])`; conflicting argument order for the same concept invites mistakes (`list/pkg.generated.mbti:24`, `list/pkg.generated.mbti:70`). + +**Suggestions** +- Flip `List::findi`’s callback signature to `(Int, A)` (or add an overload) so index-first conventions stay uniform across the API. +- Align head-insertion helpers: either deprecate `prepend` in favor of `cons`, or rename/reshape it so both functions share the same argument order and semantics. diff --git a/scripts/output/math.review.md b/scripts/output/math.review.md new file mode 100644 index 000000000..48624e597 --- /dev/null +++ b/scripts/output/math.review.md @@ -0,0 +1,21 @@ +# Review: moonbitlang/core/math + +**File:** `math/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:03.482Z +**Status:** ✓ Success + +--- + +**API Assessment** +- `math/pkg.generated.mbti` exposes a familiar, C-style math surface with broad Double/Float coverage for transcendental operations, plus bigint utilities; the optional `#as_free_fn` markers and deprecation tags signal thoughtful migration paths. + +**Issues** +- `math/pkg.generated.mbti:80` `log10` and `math/pkg.generated.mbti:82` `log2` only accept `Double`, whereas neighboring APIs ship paired `Double`/`Float` variants; this asymmetry forces callers working in `Float` to promote values. +- `math/pkg.generated.mbti:45`, `math/pkg.generated.mbti:64`, `math/pkg.generated.mbti:100`, `math/pkg.generated.mbti:123` expose `ceil`/`floor`/`round`/`trunc` only for `Double`, again breaking the otherwise consistent dual-precision story. +- `math/pkg.generated.mbti:70` `is_probable_prime` accepts an optional `iters` but `math/pkg.generated.mbti:97` `probable_prime` does not let callers tune iterations; users needing deterministic control must reimplement logic. +- The deprecation of `maximum`/`minimum` (`math/pkg.generated.mbti:85`, `math/pkg.generated.mbti:88`) points to replacements that are not visible in this interface, leaving unclear guidance for migrating callers. + +**Suggestions** +- Add `log10f`, `log2f`, and float versions of the rounding helpers to keep parity with the rest of the Float surface. +- Either mirror the `iters? : Int` knob on `probable_prime` or document the default iteration strategy to avoid surprises in cryptographic contexts. +- Surface recommended replacements for the deprecated `maximum`/`minimum` (and confirm whether `const PI` fully replaces `let pi`) so users have a clear migration path. diff --git a/scripts/output/option.review.md b/scripts/output/option.review.md new file mode 100644 index 000000000..7db7d9cfd --- /dev/null +++ b/scripts/output/option.review.md @@ -0,0 +1,19 @@ +# Review: moonbitlang/core/option + +**File:** `option/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:13.802Z +**Status:** ✓ Success + +--- + +**API Assessment** +- Overall mirrors familiar Option patterns: `map`, `bind`, `filter`, `iter`, and `unwrap_or*` cover common workflows without surprising signatures (`option/pkg.generated.mbti:12-34`). +- Nullable + `raise?` annotations make error propagation explicit and consistent across combinators. + +**Potential Issues** +- `Option::or_error(T?, Err) -> T raise Err` forces eager construction of the error value even when the option is `some`; it can be expensive or stateful (`option/pkg.generated.mbti:31-32`). +- Deprecations (`empty`, `some`, `or`, `or_else`, etc.) remain exposed in the interface and may confuse users about the preferred surface (`option/pkg.generated.mbti:9-29`). + +**Suggestions** +- Consider adding a lazy variant such as `or_error_else( () -> Err )` to defer error creation until it is actually needed. +- Move or clearly document deprecated helpers in `deprecated.mbt` so downstream users know they are legacy and what to use instead. diff --git a/scripts/output/prelude.review.md b/scripts/output/prelude.review.md new file mode 100644 index 000000000..7d4f5313f --- /dev/null +++ b/scripts/output/prelude.review.md @@ -0,0 +1,23 @@ +# Review: moonbitlang/core/prelude + +**File:** `prelude/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:42.411Z +**Status:** ✓ Success + +--- + +**API Assessment** +- `prelude/pkg.generated.mbti` offers a concise grab bag of control helpers (`abort`, `panic`, `fail`, `assert_*`) and re-exports that make the rest of the standard library easier to consume. +- The fluent helpers (`tap`, `then`, `ignore`) at `prelude/pkg.generated.mbti:47` feel coherent with functional chaining patterns already present elsewhere. +- Re-exporting builtin aliases keeps client code short and mirrors the expectations of a “prelude” layer. + +**Issues** +- `prelude/pkg.generated.mbti:39` (`panic`) returns `T` without any `raise` annotation or payload, so effect tracking tools can’t distinguish it from a pure value-returning function and callers get no diagnostic context. +- `prelude/pkg.generated.mbti:9`, `:27`, and `:39` expose three failure-oriented APIs (`abort`, `fail`, `panic`) whose semantics are hard to differentiate from the signatures alone; only `fail` communicates its failure mode. +- `prelude/pkg.generated.mbti:33` takes `&@builtin.Show`, whereas the neighbouring `println`/`repr` use a typeclass constraint; mixing explicit trait objects with implicit constraints in a tiny surface stands out as an inconsistency. +- `prelude/pkg.generated.mbti:41` exports `physical_equal` for all `T`, but value semantics for many `T` make physical identity undefined; without documentation or a trait bound it invites misuse. + +**Suggestions** +- Make `panic` explicit—either drop the return value or mark it `-> T raise @builtin.Failure` (or whatever it actually raises) and consider an optional message parameter. +- Clarify or consolidate the failure helpers: document each via naming (`panic_with` vs `abort_process`) or reduce to one primary entry point with variants for message/location. +- Align `inspect` with the rest of the API by switching to a typeclass constraint (`T : Show`) and leveraging implicit evidence, or otherwise document why a by-ref trait object is required. diff --git a/scripts/output/priority_queue.review.md b/scripts/output/priority_queue.review.md new file mode 100644 index 000000000..61ececfab --- /dev/null +++ b/scripts/output/priority_queue.review.md @@ -0,0 +1,13 @@ +# Review: moonbitlang/core/priority_queue + +**File:** `priority_queue/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:25.583Z +**Status:** ✓ Success + +--- + +- `priority_queue/pkg.generated.mbti:23` `T::unsafe_pop(Self[A]) -> Unit` suggests it removes the top element without returning it, but the name implies a non-checked variant of `pop` that yields the element. Either return `A` or rename (e.g. `discard_top`) to avoid misuse. +- `priority_queue/pkg.generated.mbti:18` `T::iter(Self[A]) -> Iter[A]` is gated by `Compare`, yet iteration does not appear to need ordering; likewise `to_array` (line 21). Removing those bounds would make read-only access usable for types that implement `Equal`/`Show` but not `Compare`. +- `priority_queue/pkg.generated.mbti:17` `T::from_iter(Iter[K]) -> Self[K]` uses a different type parameter name from the rest of the API; harmless but mildly inconsistent with `A` elsewhere. + +Overall the API exposes the expected heap capabilities (construction from various containers, peek/pop/push, iteration, conversions) and derives helpful traits (`Default`, `Show`, `ToJson`, QuickCheck support). Tidying the points above would improve clarity and ergonomics without altering behavior. diff --git a/scripts/output/queue.review.md b/scripts/output/queue.review.md new file mode 100644 index 000000000..9573e3b65 --- /dev/null +++ b/scripts/output/queue.review.md @@ -0,0 +1,15 @@ +# Review: moonbitlang/core/queue + +**File:** `queue/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:31.498Z +**Status:** ✓ Success + +--- + +**API Review** +- `queue/pkg.generated.mbti:23` and `queue/pkg.generated.mbti:24` expose `unsafe_peek` / `unsafe_pop` that behave like the deprecated `*_exn` variants but aren’t themselves deprecated; this prolongs the “panic-on-empty” path instead of steering users to the safe optional-returning API. +- `queue/pkg.generated.mbti:29` publishes `typealias Queue as T`, introducing a highly-generic exported name that’s easy to collide with in downstream code and obscures the actual type being used. + +**Suggestions** +- (1) Either deprecate the `unsafe_*` helpers alongside `*_exn`, or document clearly why both sets exist; ideally keep just the option-returning accessors in the public surface. +- (2) Drop or rename the public alias `T`; if an alias is required, give it a descriptive name so downstream packages can reference the queue type explicitly. diff --git a/scripts/output/quickcheck.review.md b/scripts/output/quickcheck.review.md new file mode 100644 index 000000000..fb65563c9 --- /dev/null +++ b/scripts/output/quickcheck.review.md @@ -0,0 +1,18 @@ +# Review: moonbitlang/core/quickcheck + +**File:** `quickcheck/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:34.571Z +**Status:** ✓ Success + +--- + +**API Notes** +- Minimal surface (`gen`, `samples`, `Arbitrary`) keeps usage straightforward while covering single-sample and bulk generation needs; default implementations for primitives give good out-of-the-box coverage (`quickcheck/pkg.generated.mbti:8`, `quickcheck/pkg.generated.mbti:10`, `quickcheck/pkg.generated.mbti:15`). + +**Issues / Inconsistencies** +- `gen` accepts an optional RNG state, but `samples` does not, so bulk generation cannot be seeded/replayed in the same way (`quickcheck/pkg.generated.mbti:8`, `quickcheck/pkg.generated.mbti:10`). +- `Arbitrary` requires passing `@splitmix.RandomState`, leaking the internal RNG choice to every implementer; this couples third-party generators to the concrete engine and makes future RNG swaps breaking (`quickcheck/pkg.generated.mbti:15`). + +**Improvements** +1. Add a seeded overload (or optional parameter) for `samples` so sampling and single generation share the same control surface. +2. Re-export `RandomState` or wrap it in a package-level type/trait so implementers are insulated from the concrete RNG and migrations stay source-compatible. diff --git a/scripts/output/quickcheck_splitmix.review.md b/scripts/output/quickcheck_splitmix.review.md new file mode 100644 index 000000000..f2276717d --- /dev/null +++ b/scripts/output/quickcheck_splitmix.review.md @@ -0,0 +1,25 @@ +# Review: moonbitlang/core/quickcheck/splitmix + +**File:** `quickcheck/splitmix/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:42.890Z +**Status:** ✓ Success + +--- + +**API Design** +- `quickcheck/splitmix/pkg.generated.mbti:5` Top-level `new(seed?)` plus `Default` give an easy entry point and optional seeding matches common PRNG APIs; method set covers ints/floats plus splitting. +- `quickcheck/splitmix/pkg.generated.mbti:11-23` Consistent receiver-taking-by-value style suggests `RandomState` is copy-on-write; `Show` impl aids debugging. + +**Issues** +- `quickcheck/splitmix/pkg.generated.mbti:14` A method named `next` that returns `Unit` is surprising—callers expect a value; likely either misnamed (e.g. `advance`) or should return the generated number. +- `quickcheck/splitmix/pkg.generated.mbti:19` `next_positive_int` promises positivity yet returns `Int`; users may expect `UInt` or at least clarity on whether zero can appear. +- `quickcheck/splitmix/pkg.generated.mbti:20` `next_two_uint` returning a tuple while other methods return scalars is asymmetrical; unclear why a single-call pair is special but other paired outputs (e.g. doubles) are absent. +- `quickcheck/splitmix/pkg.generated.mbti:5` + `13` Redundant constructors: keeping the free `new` while deprecating the method leaves surface area confusion; consumers may wonder which to use and when the deprecated entry will disappear. + +**Suggestions** +- `quickcheck/splitmix/pkg.generated.mbti:14` Either rename to `advance` (if it only steps state) or return the same type as `next_uint64`; document behavior. +- `quickcheck/splitmix/pkg.generated.mbti:19` Clarify contract (e.g. rename to `next_nonzero_int`/`next_positive_uint`) or adjust return type to `UInt`. +- `quickcheck/splitmix/pkg.generated.mbti:20` Consider exposing symmetric helpers (e.g. `next_uintN`, `next_double_pair`) or clarifying why this pair method exists; otherwise keep surface minimal. +- `quickcheck/splitmix/pkg.generated.mbti:13` Plan to remove the deprecated `RandomState::new` soon or provide migration notes so the API surface stays tidy. + +Potential next step: add doc comments in the implementation package to explain the intent behind `next`, `next_positive_int`, and `next_two_uint`. diff --git a/scripts/output/random.review.md b/scripts/output/random.review.md new file mode 100644 index 000000000..ddd5aab27 --- /dev/null +++ b/scripts/output/random.review.md @@ -0,0 +1,20 @@ +# Review: moonbitlang/core/random + +**File:** `random/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:44.720Z +**Status:** ✓ Success + +--- + +**API Assessment** +- `Rand` offers a reasonably complete RNG surface (ints, floats, bigint, shuffle) with a pluggable `Source`, which keeps the type reusable. +- Static factories (`Rand::chacha8`, `Rand::new`) plus the `Source` trait give users both high- and low-level control; optional arguments keep defaults handy. + +**Potential Issues** +- Deprecated free functions `chacha8`/`new` still appear in the surface; unless there is a compatibility window, their presence may confuse users wondering which entrypoint to prefer. +- `Rand::chacha8` and the deprecated `chacha8` differ in return type (`Rand` vs `&Source`). If callers migrate naively, behaviour changes; worth calling out in docs or via naming. +- `Rand::shuffle(Self, Int, (Int, Int) -> Unit)` exposes a non-obvious callback signature; without docs it’s unclear how indices map to collection operations. + +**Suggestions** +- Consider fully removing or hiding deprecated functions once migration is complete, or add doc comments in the interface clarifying the new factory to minimise ambiguity. +- If `shuffle` is kept, document the callback contract (e.g. expects a swap closure) or provide container-specific helpers so users don’t have to decode the callback shape. diff --git a/scripts/output/random_internal_random_source.review.md b/scripts/output/random_internal_random_source.review.md new file mode 100644 index 000000000..df30892a5 --- /dev/null +++ b/scripts/output/random_internal_random_source.review.md @@ -0,0 +1,19 @@ +# Review: moonbitlang/core/random/internal/random_source + +**File:** `random/internal/random_source/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:48.506Z +**Status:** ✓ Success + +--- + +**Findings** +- random/internal/random_source/pkg.generated.mbti:9 — `ChaCha8::next(Self) -> UInt64?` signals exhaustion via `None`, yet the only recovery hook is the separate `refill` method; the contract between these two calls remains opaque to API users, making misuse (e.g., ignoring `None` or redundant `refill` calls) likely. +- random/internal/random_source/pkg.generated.mbti:10 — `ChaCha8::refill(Self) -> Unit` is exposed publicly but looks like an implementation detail; surfacing it without documentation or trait integration encourages leaking internal buffering mechanics into callers. + +**Assessment** +- Minimal surface: a single RNG type with manual replenishment. +- Lacks seeding clarity; `ChaCha8::new(Bytes)` leaves required length/entropy unspecified, so type safety around keys/nonces is weak. + +**Suggestions** +- Document or encode the seed requirements (e.g., dedicated seed type or length-checked constructor). +- Either hide `refill` or wrap it so `next` transparently handles buffering; alternatively expose a trait-based interface (e.g., `RandomSource::next_u64`) to simplify usage expectations. diff --git a/scripts/output/rational.review.md b/scripts/output/rational.review.md new file mode 100644 index 000000000..4ece558ba --- /dev/null +++ b/scripts/output/rational.review.md @@ -0,0 +1,16 @@ +# Review: moonbitlang/core/rational + +**File:** `rational/pkg.generated.mbti` +**Date:** 2025-10-09T10:00:44.129Z +**Status:** ✓ Success + +--- + +- Heavily deprecated surface: Every exposed constructor and method in `rational/pkg.generated.mbti` is flagged `#deprecated`, so the public type `T` effectively lacks a stable way to be created or manipulated. +- Missing migration path: The interface doesn’t advertise replacement APIs, making it unclear how clients should build rationals or which new module supersedes this one. +- Mixed error conventions: `from_double` uses an error-raising result while `new` returns `T?`, hinting at inconsistent error-handling semantics that a replacement API should settle. + +Suggestions: +1. Publish the intended replacement constructors/converters (e.g., `try_new`, `from_int`, `from_string`) and deprecate only after they land. +2. Document—or better, encode in types—the preferred error strategy so callers know whether to expect `raise` vs option. +3. If the package is truly retired, add a top-level note directing users to the successor module to prevent confusion. diff --git a/scripts/output/ref.review.md b/scripts/output/ref.review.md new file mode 100644 index 000000000..827de8527 --- /dev/null +++ b/scripts/output/ref.review.md @@ -0,0 +1,19 @@ +# Review: moonbitlang/core/ref + +**File:** `ref/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:00.491Z +**Status:** ✓ Success + +--- + +**Potential Issues** +- `ref/pkg.generated.mbti:8` – `Ref::protect(Self[T], T, () -> R raise?) -> R raise?` is hard to parse without docs; the extra `T` parameter (presumably a default/reset value) is non-obvious and duplicating the current value by copy may be surprising for consumers. +- `ref/pkg.generated.mbti:5` – Exposing both `fn[T] new(T) -> Ref[T]` and `Ref::new` looks redundant; unless there’s a convention-driven need for the free function, it risks API surface bloat. + +**Assessment** +- Overall the API is small and focused on mutable reference utilities with functional-style helpers (`map`, `update`, `swap`), which reads clean and generic-friendly. +- The use of `raise?` on the higher-order helpers is consistent, signalling that user callbacks may raise, and the `quickcheck.Arbitrary` instance is a nice integration touch. + +**Suggestions** +1. Clarify or rethink `Ref::protect`: if the second `T` argument is essential, consider renaming/adding docs or refactoring to an explicit helper so its intent isn’t ambiguous. +2. Decide whether both `new` entry points are needed; if not, consolidate on either the free function or the associated function to simplify the API. diff --git a/scripts/output/result.review.md b/scripts/output/result.review.md new file mode 100644 index 000000000..6dff6890c --- /dev/null +++ b/scripts/output/result.review.md @@ -0,0 +1,20 @@ +# Review: moonbitlang/core/result + +**File:** `result/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:15.965Z +**Status:** ✓ Success + +--- + +**API Review** +- Public surface is unsurprising for a `Result` monad: bind/flatten/map/map_err plus rich unwrap family cover most ergonomics, and Compare/Arbitrary impls align with common expectations. + +**Findings** +- `result/pkg.generated.mbti:10` `err` and `result/pkg.generated.mbti:13` `ok` are deprecated without an obvious replacement exposed in this interface, so consumers may be left guessing about the preferred constructor path. +- `result/pkg.generated.mbti:28` `Result::or(Self[T, E], T) -> T` reuses a name that usually returns another `Result`; returning the plain `T` duplicates `unwrap_or` but with a misleading label. Same concern for `result/pkg.generated.mbti:29` `or_else`, which mirrors `unwrap_or_else` but drops the `raise?` capability. +- `result/pkg.generated.mbti:31` `unwrap` and `result/pkg.generated.mbti:32` `unwrap_err` lack a `raise` effect in the signature; if they can abort on `Err`, the type should advertise it for consistency with `unwrap_or_error`. + +**Suggestions** +- Provide or document the modern constructors (e.g., new block with recommended `Result.ok`/`Result.err` replacements) so the deprecation path is clear. +- Consider renaming `Result::or`/`or_else` to something that conveys their `unwrap` semantics, or alternatively return `Self` to match conventional `or` behavior. +- If `unwrap`/`unwrap_err` can raise/panic, mark them with the proper `raise` effect; if they cannot, clarify their guarantees in documentation to reassure users. diff --git a/scripts/output/set.review.md b/scripts/output/set.review.md new file mode 100644 index 000000000..cde8ced0d --- /dev/null +++ b/scripts/output/set.review.md @@ -0,0 +1,20 @@ +# Review: moonbitlang/core/set + +**File:** `set/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:27.579Z +**Status:** ✓ Success + +--- + +**Assessment** +- Method set in `set/pkg.generated.mbti:9-41` looks cohesive: it covers core mutating and set-algebra operations plus conversions, mirroring what users expect from a standard set. +- Free-function constructors (`from_array`, `from_iter`, `new`, `of`) at `set/pkg.generated.mbti:19-36` give flexible creation paths without overloading the main type. +- Trait impl coverage at `set/pkg.generated.mbti:42-49` is rich (bitwise ops, `Eq`, `Show`, `Default`, `ToJson`), so the type composes well with the wider ecosystem. + +**Issues** +- `Set::eachi` at `set/pkg.generated.mbti:18` exposes an index even though set iteration is typically unordered; without a documented ordering guarantee the index may mislead users into assuming determinism. +- The alias annotation `#alias(size, deprecated)` on `Set::length` at `set/pkg.generated.mbti:31-32` surfaces only the deprecated name to interface consumers, which makes it unclear whether `length` or `size` is the preferred, non-deprecated entry point. + +**Suggestions** +- Consider clarifying `Set::eachi`’s index semantics (e.g., document that the index is arbitrary or remove the index if it has no semantic meaning). +- Adjust the alias so that the non-deprecated `length` name is the primary symbol exposed in the generated interface, leaving `size` as the hidden deprecated alias. diff --git a/scripts/output/sorted_map.review.md b/scripts/output/sorted_map.review.md new file mode 100644 index 000000000..2d319515e --- /dev/null +++ b/scripts/output/sorted_map.review.md @@ -0,0 +1,22 @@ +# Review: moonbitlang/core/sorted_map + +**File:** `sorted_map/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:24.852Z +**Status:** ✓ Success + +--- + +**Assessment** +- Public surface covers the expected Map basics plus ordered-friendly APIs (iterators, range) with consistent `Compare` bounds for order-sensitive operations (`sorted_map/pkg.generated.mbti:16-45`). +- Free-function aliases for constructors (`from_array`, `from_iter`, `of`, `new`) make creation ergonomic in multiple contexts (`sorted_map/pkg.generated.mbti:22-37`). +- Trait coverage (Default/Eq/Show/ToJson/QuickCheck) looks thoughtfully composed for both runtime and testing scenarios (`sorted_map/pkg.generated.mbti:47-52`). + +**Potential Issues** +- `SortedMap::remove(Self, K) -> Unit` offers no feedback on whether the key existed (`sorted_map/pkg.generated.mbti:39-42`); users must pre-check with `contains`/`get`. +- `SortedMap::range(Self, K, K) -> Iter2[K, V]` exposes order slices but the interface gives no hint about bound inclusivity/exclusivity (`sorted_map/pkg.generated.mbti:38`), which could lead to misuse without consulting docs. +- Deprecated array-returning helpers remain exported (`keys`, `values`, alias `add`, alias `size`) (`sorted_map/pkg.generated.mbti:29-45`); without steering consumers toward iter-based replacements, they may linger in new code. + +**Suggestions** +- Consider Returning an `Option[V]` or `Bool` from `remove` so callers can react without an extra lookup (or document the current silent behavior more clearly). +- Clarify `range` semantics via naming (`range_inclusive`, `range_open`) or accompanying docs/comments to reduce ambiguity. +- Reinforce deprecation by documenting preferred replacements (`keys_as_iter`, `values_as_iter`, `set`) in the main docs or by relocating legacy APIs to a dedicated deprecated module to discourage new usage. diff --git a/scripts/output/sorted_set.review.md b/scripts/output/sorted_set.review.md new file mode 100644 index 000000000..bffb9e66f --- /dev/null +++ b/scripts/output/sorted_set.review.md @@ -0,0 +1,21 @@ +# Review: moonbitlang/core/sorted_set + +**File:** `sorted_set/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:19.158Z +**Status:** ✓ Success + +--- + +**Assessment** +- The surface in `sorted_set/pkg.generated.mbti` covers the expected ordered-set operations (construction, membership, set algebra, iteration) and keeps mutation-only functions (`add`, `remove`) separate from the pure ones, which makes the API easy to reason about. +- Generic constraints are consistently applied only where ordering is required, so callers aren’t forced to supply a comparator when it isn’t needed. + +**Issues** +- `SortedSet::length` is marked `#alias(size, deprecated)` but the canonical `size` entry isn’t visible in the interface; this can confuse users and tooling that expect the non-deprecated name. +- Both `copy` and the deprecated `deep_clone` remain exported; without docs, it’s unclear whether `copy` is always deep (as the new name suggests) or whether there’s still a behavioral distinction. +- Several deprecations (`diff`, `intersect`, `deep_clone`, `length`) persist in the main namespace rather than being isolated (e.g., in `deprecated.mbt`), which keeps clutter in the primary API view. + +**Suggestions** +- Ensure the canonical `size` signature is emitted (or remove the alias marker) so the preferred name is obvious to callers. +- If `copy` fully replaces `deep_clone`, consider moving the deprecated symbols into a dedicated `deprecated.mbt` file or documenting the intended transition inline. +- Add brief documentation or naming adjustments clarifying whether `copy` performs a deep copy to avoid ambiguity after deprecating `deep_clone`. diff --git a/scripts/output/strconv.review.md b/scripts/output/strconv.review.md new file mode 100644 index 000000000..3d0602b3e --- /dev/null +++ b/scripts/output/strconv.review.md @@ -0,0 +1,21 @@ +# Review: moonbitlang/core/strconv + +**File:** `strconv/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:16.819Z +**Status:** ✓ Success + +--- + +**API Review** +- Balanced surface: covers primitives plus a generic `parse` (`strconv/pkg.generated.mbti:5`); `StrConvError` is coherently exposed with `Show`. +- Deprecated Decimal block is still exposed but every constructor/helper is deprecated (`strconv/pkg.generated.mbti:21`), leaving an odd partially supported type. + +Potential issues & inconsistencies +- Mixed input types: most `parse_*` take `StringView`, but `parse` and `FromStr::from_string` use `String`, which may force unnecessary allocations or conversions (`strconv/pkg.generated.mbti:5`, `strconv/pkg.generated.mbti:36`). +- `Decimal` itself isn’t marked `#deprecated` even though every associated function is, so consumers may not realize the whole type is legacy (`strconv/pkg.generated.mbti:21`). +- `Decimal::shift(Self, Int) -> Unit` suggests mutation on a value parameter; without a reference type this likely discards the updated value, making the API hard to use (`strconv/pkg.generated.mbti:27`). + +Suggestions +- 1) Align string inputs on either `StringView` or `String` (ideally `StringView`) across the API, including `FromStr::from_string`. +- 2) Deprecate the `Decimal` type itself or provide a non-deprecated alternative, clarifying migration paths. +- 3) Revisit `Decimal::shift`; return the shifted `Decimal` (or mutate via `&mut`-style API if available) to avoid no-op semantics. diff --git a/scripts/output/string.review.md b/scripts/output/string.review.md new file mode 100644 index 000000000..32b1d0a95 --- /dev/null +++ b/scripts/output/string.review.md @@ -0,0 +1,21 @@ +# Review: moonbitlang/core/string + +**File:** `string/pkg.generated.mbti` +**Date:** 2025-10-09T10:02:22.765Z +**Status:** ✓ Success + +--- + +**Findings** +- `string/pkg.generated.mbti:26-27` + `string/pkg.generated.mbti:80-81`: Exposing both `get` (returning `Int?`) and `get_char` (returning `Char?`) on `String`/`StringView` without clear naming makes it hard to tell when callers receive a Unicode scalar code vs. an actual `Char`. The overlap invites misuse; consider renaming (`get_charcode`) or clarifying docs. +- `string/pkg.generated.mbti:57-62`: All `String::trim*` helpers yield `StringView`, whereas most other transforming APIs on `String` (e.g., `replace`, `pad_*`, `repeat`) return an owned `String`. The mixed ownership story is surprising and can lead to accidental borrowing of short-lived temporaries unless the view semantics are very prominent. +- `string/pkg.generated.mbti:95`: `StringView::repeat` returns `Self`, while other allocating operations on `StringView` (`pad_*`, `replace`, etc.) return `String`. If `repeat` allocates, returning a view is inconsistent; if it doesn’t allocate, it’s unclear how the repetition is represented. The contract needs clarification or alignment. + +**Assessment** +- Broad, coherent coverage of searching, iteration, slicing, and mutation-like helpers across both `String` and `StringView`. +- Alias annotations and the `ToStringView` trait make migration paths and conversions discoverable. + +**Suggestions** +1. Harmonize the char-access APIs—either collapse to one canonical name or document the differing return types explicitly. +2. Consider offering owned variants (`trim_owned`) or tightening docs so developers know `trim*` keeps borrowing the original storage. +3. Align `StringView::repeat`’s return type with other allocating helpers (most likely `String`) or document how it safely returns a view. diff --git a/scripts/output/string_regex.review.md b/scripts/output/string_regex.review.md new file mode 100644 index 000000000..f4d8c26f9 --- /dev/null +++ b/scripts/output/string_regex.review.md @@ -0,0 +1,19 @@ +# Review: moonbitlang/core/string/regex + +**File:** `string/regex/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:37.257Z +**Status:** ✓ Success + +--- + +API feels intentionally minimal: `compile` produces a reusable `Regex`, and `MatchResult` exposes the matched slice plus surrounding context and capture lookups. That should cover the common “compile once, execute many times” workflow. + +Potential issues: +- `MatchResult::named_group(Self, String)` accepts `String` while every other argument is a `StringView`. That mismatch could lead to needless allocations or call-site friction. +- `compile` is marked `raise`, but the interface doesn’t clarify what error type(s) appear. Consumers can’t pattern-match specific failures. +- Only a single-match `Regex::execute` is exposed; if multi-match iteration exists elsewhere, it isn’t visible here. + +Improvements to consider: +1. Align argument types (`StringView` vs `String`) across the API to avoid surprises. +2. Document or expose a concrete error type for `compile`, or return a `Result` so callers can handle failures predictably. +3. If supported, expose an iterator-style search (e.g., `find_all` or `next_match`) so users don’t have to re-slice strings manually. diff --git a/scripts/output/string_regex_internal_regex_impl.review.md b/scripts/output/string_regex_internal_regex_impl.review.md new file mode 100644 index 000000000..f6e82abb2 --- /dev/null +++ b/scripts/output/string_regex_internal_regex_impl.review.md @@ -0,0 +1,12 @@ +# Review: moonbitlang/core/string/regex/internal/regex_impl + +**File:** `string/regex/internal/regex_impl/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:44.602Z +**Status:** ✓ Success + +--- + +**API Review** +- **Assessment**: Compact surface with a single constructor (`compile`) and a small `Regexp` method set (`string/regex/internal/regex_impl/pkg.generated.mbti:5`, `string/regex/internal/regex_impl/pkg.generated.mbti:44-50`). `MatchResult` helpers cover common queries (`before`, `after`, `groups`, `matched`) (`string/regex/internal/regex_impl/pkg.generated.mbti:36-41`), so the high-level flow feels complete. +- **Issues**: Publicly exposing the entire `MatchResult` struct makes internal bookkeeping (`captures`, `names`) part of the ABI (`string/regex/internal/regex_impl/pkg.generated.mbti:29-35`). Argument types mix `StringView` and `String`; `Regexp::group_by_name`/`group_names` return owning `String`, unlike the rest of the API (`string/regex/internal/regex_impl/pkg.generated.mbti:47-49`). `Regexp::execute` always returns a `MatchResult` even on failure, whereas `match_` is optional; the semantic distinction is unclear (`string/regex/internal/regex_impl/pkg.generated.mbti:44-50`). Deprecated `execute_with_remainder` lingers without a replacement path (`string/regex/internal/regex_impl/pkg.generated.mbti:46`). +- **Suggestions**: Consider keeping `MatchResult` internals private and exposing only accessor methods, so representation changes stay compatible. Standardize parameters and return types on `StringView` (or introduce a dedicated flag type) for consistency. Clarify or consolidate `execute` vs `match_`—perhaps have one return an option and document behavior—or provide naming that communicates their difference. If `execute_with_remainder` is obsolete, move it to your `deprecated.mbt` or document its successor to guide users. diff --git a/scripts/output/string_regex_internal_unicode.review.md b/scripts/output/string_regex_internal_unicode.review.md new file mode 100644 index 000000000..8f541eea0 --- /dev/null +++ b/scripts/output/string_regex_internal_unicode.review.md @@ -0,0 +1,18 @@ +# Review: moonbitlang/core/string/regex/internal/unicode + +**File:** `string/regex/internal/unicode/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:34.628Z +**Status:** ✓ Success + +--- + +**Findings** +- string/regex/internal/unicode/pkg.generated.mbti:9 High – `general_category_property_value_alises` is misspelled (`alises` vs `aliases`), so downstream users must rely on an incorrect identifier. +- string/regex/internal/unicode/pkg.generated.mbti:7 Medium – `case_folding : Map[Char, Char]` only supports single-codepoint folds; Unicode simple folds are fine, but full case folding and Turkic special cases need multi-codepoint targets, so the type may be too restrictive if broader folding is ever required. + +**Assessment** +- Surface is minimal and consistent with an internal lookup module, exposing constants and lookup tables only. Using `Map` and `Array` keeps the interface simple but leaks implementation choices (e.g., arrays for ranges rather than structured pairs). + +**Suggestions** +1. Rename `general_category_property_value_alises` to `general_category_property_value_aliases` and regenerate the interface. +2. If full Unicode support is a goal, consider changing `case_folding` to map to a sequence type (`String` or `Array[Char]`) and documenting whether only simple folds are provided. diff --git a/scripts/output/test.review.md b/scripts/output/test.review.md new file mode 100644 index 000000000..cd8854dd7 --- /dev/null +++ b/scripts/output/test.review.md @@ -0,0 +1,18 @@ +# Review: moonbitlang/core/test + +**File:** `test/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:51.088Z +**Status:** ✓ Success + +--- + +- API still reads like a thin testing DSL, but the public surface mixes current and deprecated helpers, which makes it feel cluttered and harder to know what to use (`test/pkg.generated.mbti:5`, `:23`). +- `is_not` only bounds `Show`, yet it must compare two values; lacking `Eq` feels inconsistent with `eq`/`ne` and may hide type errors (`test/pkg.generated.mbti:17`). +- Free function `new` constructs `T` but is not namespaced as `T::new`, unlike the rest of the struct API, so discovery/composability suffers (`test/pkg.generated.mbti:27`). +- Snapshot helper requires both `loc` and `args_loc` autofill attributes, which is consistent, but `same_object` needlessly requires `Show` just to check identity (`test/pkg.generated.mbti:30`). +- Deprecated methods on `T` (e.g., `T::bench`) remain exposed; consider moving them to `deprecated.mbt` to align with the project convention (`test/pkg.generated.mbti:36`). + +Suggestions: +1. Tighten trait bounds: add `Eq` to `is_not` (and drop `Show` from `same_object` if not needed). +2. Promote a `T::new` constructor (or alias) and discourage the free `new`. +3. Segregate deprecated helpers into `deprecated.mbt` so consumers see a cleaner core API. diff --git a/scripts/output/tuple.review.md b/scripts/output/tuple.review.md new file mode 100644 index 000000000..b85f8d64c --- /dev/null +++ b/scripts/output/tuple.review.md @@ -0,0 +1,15 @@ +# Review: moonbitlang/core/tuple + +**File:** `tuple/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:59.590Z +**Status:** ✓ Success + +--- + +**Findings** +- `tuple/pkg.generated.mbti:7-33` exposes only deprecated tuple helpers (`curry`, `map_*`, `pair`, `swap`, etc.), but there are no newer alternatives in the interface, so callers still rely on APIs that you intended to retire. Consider either removing them from the public surface or surfacing a recommended replacement before deprecating. +- `tuple/pkg.generated.mbti:36-66` implements `Default` for tuples of arity 2–16, yet `@quickcheck.Arbitrary` only exists up to arity 7. The mismatch makes tuple sizes ≥8 impossible to generate via QuickCheck even though `Default` supports them. If this is accidental, extend `Arbitrary`; if intentional, document the limit so users don’t assume broader coverage. + +**Suggestions** +1. Decide whether to publish non-deprecated tuple helpers (or move the old ones into a `deprecated` package) so consumers know what to use instead. +2. Align trait coverage—either add `@quickcheck.Arbitrary` implementations through 16 elements or add notes explaining why arity 8+ are unsupported. diff --git a/scripts/output/uint.review.md b/scripts/output/uint.review.md new file mode 100644 index 000000000..57d85a930 --- /dev/null +++ b/scripts/output/uint.review.md @@ -0,0 +1,17 @@ +# Review: moonbitlang/core/uint + +**File:** `uint/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:50.058Z +**Status:** ✓ Success + +--- + +**Findings** +- `uint/pkg.generated.mbti:3-4` exposes both a free `default()` function and an `impl Default for UInt`; this redundancy can confuse consumers about the canonical zero-value constructor. +- `uint/pkg.generated.mbti:8-9` offers `to_be_bytes` / `to_le_bytes` without corresponding `from_*` constructors, making the API asymmetric and forcing users to reimplement parsing from bytes. +- `uint/pkg.generated.mbti:10` only provides `to_int64`; callers needing other integer widths (e.g., `Int32`, `UInt32`) or arbitrary-precision conversion must implement conversions themselves, reducing usability. + +**Suggestions** +- Retire the standalone `default()` in favor of the trait-based entry point, or clearly document why both exist. +- Add mirrored `from_be_bytes` / `from_le_bytes` (and potentially a generic `from_bytes` with endianness parameter) to round out the serialization surface. +- Consider a small suite of integer conversion helpers (`to_int32`, `try_to_int`, `from_int64`, etc.) so consumers can avoid manual range checks and casts. diff --git a/scripts/output/uint16.review.md b/scripts/output/uint16.review.md new file mode 100644 index 000000000..e77c02b3d --- /dev/null +++ b/scripts/output/uint16.review.md @@ -0,0 +1,19 @@ +# Review: moonbitlang/core/uint16 + +**File:** `uint16/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:56.441Z +**Status:** ✓ Success + +--- + +Solid selection of conversions and logic helpers; the public surface in `uint16/pkg.generated.mbti:1` feels cohesive and covers the expected arithmetic/bit traits plus JSON support. + +Potential issues +- `UInt16::reinterpret_as_int16` (`uint16/pkg.generated.mbti:11`) exposes a lossy reinterpretation with no safe alternative; the name implies “bit cast”, yet the result is signed and may surprise callers with negative values. +- The surrogate helpers (`uint16/pkg.generated.mbti:7-9`) imply UTF-16 awareness, but there’s no complementary constructor (e.g., from `Char`) or documentation hint, so consumers may struggle to use them correctly. +- Only `unsafe_to_char` (`uint16/pkg.generated.mbti:15`) throws on invalid values; lacking an intermediate `expect_*`-style helper or explicit error type may lead to panics in user code. + +Suggestions +1. Rename `reinterpret_as_int16` (e.g., `to_int16_wrapping`) or add a checked `to_int16()` that returns an option/result to clarify intent. +2. Introduce a `from_char(Char) -> UInt16` (and possibly surrogate-specific constants) to round out the Unicode helpers. +3. Consider documenting or encoding failure modes for `unsafe_to_char`, either via a safe wrapper or by signalling the panic contract in the interface docs. diff --git a/scripts/output/uint64.review.md b/scripts/output/uint64.review.md new file mode 100644 index 000000000..111e11f0d --- /dev/null +++ b/scripts/output/uint64.review.md @@ -0,0 +1,17 @@ +# Review: moonbitlang/core/uint64 + +**File:** `uint64/pkg.generated.mbti` +**Date:** 2025-10-09T10:02:18.024Z +**Status:** ✓ Success + +--- + +**Findings** +- `uint64/pkg.generated.mbti:12` Only exposes `UInt64::to_be_bytes`/`to_le_bytes`; without matching `from_{be,le}_bytes` (or a fallible constructor taking `Bytes`), clients cannot round-trip values without reimplementing parsing. + +**Assessment** +- Surface is minimal and predictable: `min_value`/`max_value` constants plus endian-specific byte dumps. Fits a low-level core module, though it skews toward serialization helpers only. + +**Suggestions** +- Add `from_be_bytes`/`from_le_bytes` (ideally fallible) to complete the endian story. +- Consider a native-endian helper (`to_ne_bytes`) for ergonomic parity with other languages’ standard libraries. diff --git a/scripts/output/unit.review.md b/scripts/output/unit.review.md new file mode 100644 index 000000000..d516cff69 --- /dev/null +++ b/scripts/output/unit.review.md @@ -0,0 +1,17 @@ +# Review: moonbitlang/core/unit + +**File:** `unit/pkg.generated.mbti` +**Date:** 2025-10-09T10:01:59.141Z +**Status:** ✓ Success + +--- + +`unit/pkg.generated.mbti` looks cohesive for a standard unit type: `default()` plus `Unit::to_string`, and trait impls for `Compare`, `Default`, `Hash`. + +Findings +- `unit/pkg.generated.mbti:5` – Exposed free function `default()` duplicates `Default for Unit`; unless a historical alias is required, it’s unusual to keep both and risks diverging behavior if one changes. + +Suggestions +- If compatibility allows, consider deprecating the standalone `default()` and rely on `Default::default()` to keep the API minimal. + +Residual risk: without implementations or usage we can’t confirm `default()` stays aligned with the trait impl, so worth double-checking tests or documentation. diff --git a/scripts/package-lock.json b/scripts/package-lock.json new file mode 100644 index 000000000..98bda8f6f --- /dev/null +++ b/scripts/package-lock.json @@ -0,0 +1,20 @@ +{ + "name": "scripts", + "lockfileVersion": 3, + "requires": true, + "packages": { + "": { + "dependencies": { + "@openai/codex-sdk": "^0.46.0" + } + }, + "node_modules/@openai/codex-sdk": { + "version": "0.46.0", + "resolved": "https://registry.npmjs.org/@openai/codex-sdk/-/codex-sdk-0.46.0.tgz", + "integrity": "sha512-xCjQnuV13rDqVf3EQa/2BD38qzelOofCFVKpexkO6LVnoYaZin/MTwzSNiAR7k8DTF7AHottgqwNOB8hbYVauA==", + "engines": { + "node": ">=18" + } + } + } +} diff --git a/scripts/package.json b/scripts/package.json new file mode 100644 index 000000000..ebb63109c --- /dev/null +++ b/scripts/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "@openai/codex-sdk": "^0.46.0" + } +} diff --git a/scripts/review-mbti-files.mjs b/scripts/review-mbti-files.mjs new file mode 100755 index 000000000..c40fbd9fb --- /dev/null +++ b/scripts/review-mbti-files.mjs @@ -0,0 +1,310 @@ +#!/usr/bin/env node + +/** + * Script to review pkg.generated.mbti files using Codex SDK + * Usage: node scripts/review-mbti-files.mjs [options] + * + * Options: + * --all Review all mbti files (default) + * --changed Only review changed files in git + * --files Specific files to review (comma-separated) + * --concurrency Number of concurrent reviews (default: 5) + * --verbose Show detailed progress + */ + +import { fileURLToPath } from "url"; +import { dirname, join, relative, basename } from "path"; +import { readdir, readFile, writeFile, mkdir } from "fs/promises"; +import { Codex } from "@openai/codex-sdk"; +import { exec } from "child_process"; +import { promisify } from "util"; + +const execAsync = promisify(exec); + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); +const projectRoot = join(__dirname, ".."); +const outputDir = join(__dirname, "output"); + +// Parse command line arguments +const args = process.argv.slice(2); +const options = { + all: args.includes("--all") || !args.includes("--changed"), + changed: args.includes("--changed"), + concurrency: parseInt( + args.find((a) => a.startsWith("--concurrency="))?.split("=")[1] || "5" + ), + verbose: args.includes("--verbose"), + files: + args + .find((a) => a.startsWith("--files=")) + ?.split("=")[1] + ?.split(",") || null, +}; + +/** + * Recursively find all pkg.generated.mbti files + */ +async function findMbtiFiles(dir, results = []) { + try { + const entries = await readdir(dir, { withFileTypes: true }); + + for (const entry of entries) { + const fullPath = join(dir, entry.name); + + if (entry.isDirectory()) { + if ( + !entry.name.startsWith(".") && + entry.name !== "node_modules" && + entry.name !== "target" + ) { + await findMbtiFiles(fullPath, results); + } + } else if (entry.name === "pkg.generated.mbti") { + results.push(fullPath); + } + } + } catch (error) { + if (error.code !== "EACCES" && error.code !== "EPERM") { + console.error(`Error reading ${dir}:`, error.message); + } + } + + return results; +} + +/** + * Get changed mbti files from git + */ +async function getChangedMbtiFiles() { + try { + const { stdout } = await execAsync("git diff --name-only HEAD", { + cwd: projectRoot, + }); + const changedFiles = stdout + .split("\n") + .filter((f) => f.endsWith("pkg.generated.mbti")) + .map((f) => join(projectRoot, f)); + + if (changedFiles.length === 0) { + console.log("No changed pkg.generated.mbti files found"); + return []; + } + + return changedFiles; + } catch (error) { + console.error("Error getting changed files:", error.message); + return []; + } +} + +/** + * Save review result to file + */ +async function saveReviewToFile(result) { + try { + // Create output directory if it doesn't exist + await mkdir(outputDir, { recursive: true }); + + // Generate filename from package path + // e.g., "bytes/pkg.generated.mbti" -> "bytes.review.md" + const fileName = result.file + .replace(/\/pkg\.generated\.mbti$/, "") + .replace(/\//g, "_") + ".review.md"; + + const outputPath = join(outputDir, fileName); + + // Format the review content + const content = `# Review: ${result.package} + +**File:** \`${result.file}\` +**Date:** ${new Date().toISOString()} +**Status:** ${result.success ? "✓ Success" : "✗ Failed"} + +${result.success ? "---\n\n" + result.review : `**Error:** ${result.error}`} +`; + + await writeFile(outputPath, content, "utf-8"); + return outputPath; + } catch (error) { + console.error(`Failed to save review for ${result.file}:`, error.message); + return null; + } +} + +/** + * Review a single mbti file using Codex + */ +async function reviewMbtiFile(filePath, codex) { + const relativePath = relative(projectRoot, filePath); + + if (options.verbose) { + console.log(`Reviewing: ${relativePath}`); + } + + try { + const content = await readFile(filePath, "utf-8"); + const packageName = content.match(/package "(.+?)"/)?.[1] || "unknown"; + + const thread = codex.startThread({ skipGitRepoCheck: true }); + + const prompt = `Review this MoonBit package interface file (${relativePath}): + +\`\`\`moonbit +${content} +\`\`\` + +Please provide: +1. A brief assessment of the API design +2. Any potential issues or inconsistencies +3. Suggestions for improvement (if any) + +Keep the review concise and focused on the public API surface.`; + + const result = await thread.run(prompt); + + // Extract text from Codex SDK result - it returns an object with finalResponse + const reviewText = result?.finalResponse || JSON.stringify(result, null, 2); + + return { + file: relativePath, + package: packageName, + review: reviewText, + success: true, + }; + } catch (error) { + return { + file: relativePath, + package: "unknown", + review: null, + success: false, + error: error.message, + }; + } +} + +/** + * Process files with concurrency limit + */ +async function processConcurrently(files, processor, concurrency) { + const results = []; + const executing = []; + const startTime = Date.now(); + + for (const file of files) { + const fileStartTime = Date.now(); + + const promise = processor(file).then(async (result) => { + results.push(result); + executing.splice(executing.indexOf(promise), 1); + + // Save review to file + const outputPath = await saveReviewToFile(result); + + // Calculate timing + const fileTime = ((Date.now() - fileStartTime) / 1000).toFixed(2); + const totalTime = ((Date.now() - startTime) / 1000).toFixed(1); + const avgTime = (totalTime / results.length).toFixed(2); + const eta = ((files.length - results.length) * avgTime).toFixed(0); + + // Show progress with status indicator + const status = result.success ? "✓" : "✗"; + const savedMsg = outputPath ? ` → ${relative(projectRoot, outputPath)}` : ""; + console.log( + `${status} [${results.length}/${files.length}] ${result.file} (${fileTime}s, ETA: ${eta}s)${savedMsg}` + ); + + return result; + }); + + executing.push(promise); + + if (executing.length >= concurrency) { + await Promise.race(executing); + } + } + + await Promise.all(executing); + return results; +} + +/** + * Main function + */ +async function main() { + console.log("🔍 Starting mbti file review...\n"); + + // Determine which files to review + let filesToReview = []; + + if (options.files) { + filesToReview = options.files.map((f) => join(projectRoot, f)); + console.log(`Reviewing ${filesToReview.length} specified file(s)`); + } else if (options.changed) { + filesToReview = await getChangedMbtiFiles(); + console.log(`Found ${filesToReview.length} changed mbti file(s)`); + } else { + filesToReview = await findMbtiFiles(projectRoot); + console.log(`Found ${filesToReview.length} mbti file(s) to review`); + } + + if (filesToReview.length === 0) { + console.log("No files to review. Exiting."); + return; + } + + console.log(`Using concurrency limit: ${options.concurrency}\n`); + + // Initialize Codex + const codex = new Codex(); + + // Process files with concurrency control + const startTime = Date.now(); + const results = await processConcurrently( + filesToReview, + (file) => reviewMbtiFile(file, codex), + options.concurrency + ); + const duration = ((Date.now() - startTime) / 1000).toFixed(2); + + // Display results + console.log("\n" + "=".repeat(80)); + console.log("REVIEW RESULTS"); + console.log("=".repeat(80) + "\n"); + + const successful = results.filter((r) => r.success); + const failed = results.filter((r) => !r.success); + + successful.forEach((result, index) => { + console.log(`\n${"─".repeat(80)}`); + console.log(`📦 ${result.package} (${result.file})`); + console.log(`${"─".repeat(80)}\n`); + console.log(result.review); + }); + + // Summary + console.log("\n" + "=".repeat(80)); + console.log("SUMMARY"); + console.log("=".repeat(80)); + console.log(`Total files: ${results.length}`); + console.log(`Successful: ${successful.length}`); + console.log(`Failed: ${failed.length}`); + console.log(`Duration: ${duration}s`); + console.log(`Concurrency: ${options.concurrency}`); + console.log(`Output directory: ${relative(projectRoot, outputDir)}`); + + if (failed.length > 0) { + console.log("\n❌ Failed reviews:"); + failed.forEach((f) => { + console.log(` - ${f.file}: ${f.error}`); + }); + } + + console.log(`\n💾 Reviews saved to: ${outputDir}`); +} + +// Run the script +main().catch((error) => { + console.error("Error:", error.message); + process.exit(1); +}); diff --git a/scripts/test-codex-response.mjs b/scripts/test-codex-response.mjs new file mode 100755 index 000000000..08069fbd6 --- /dev/null +++ b/scripts/test-codex-response.mjs @@ -0,0 +1,29 @@ +#!/usr/bin/env node + +/** + * Test script to see what Codex SDK returns + */ + +import { Codex } from "@openai/codex-sdk"; + +async function test() { + console.log("Testing Codex SDK response structure...\n"); + + const codex = new Codex(); + const thread = codex.startThread({ skipGitRepoCheck: true }); + + const result = await thread.run("Say hello in one sentence."); + + console.log("Result type:", typeof result); + console.log("\nResult keys:", Object.keys(result || {})); + console.log("\nFull result:"); + console.log(JSON.stringify(result, null, 2)); + + console.log("\n--- Accessing properties ---"); + console.log("result.text:", result?.text); + console.log("result.content:", result?.content); + console.log("result.message:", result?.message); + console.log("result.response:", result?.response); +} + +test().catch(console.error); diff --git a/scripts/test-review-extraction.mjs b/scripts/test-review-extraction.mjs new file mode 100644 index 000000000..2b71f6d9f --- /dev/null +++ b/scripts/test-review-extraction.mjs @@ -0,0 +1,48 @@ +#!/usr/bin/env node + +/** + * Quick test to verify review extraction works + */ + +import { Codex } from "@openai/codex-sdk"; +import { fileURLToPath } from "url"; +import { dirname, join } from "path"; +import { readFile } from "fs/promises"; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); +const projectRoot = join(__dirname, ".."); + +async function test() { + console.log("Testing review extraction...\n"); + + const codex = new Codex(); + const thread = codex.startThread({ skipGitRepoCheck: true }); + + // Read a small package file + const filePath = join(projectRoot, "bool/pkg.generated.mbti"); + const content = await readFile(filePath, "utf-8"); + + const prompt = `Review this MoonBit package interface file (bool/pkg.generated.mbti): + +\`\`\`moonbit +${content} +\`\`\` + +Please provide a brief 2-3 sentence assessment of the API design.`; + + console.log("Sending request..."); + const result = await thread.run(prompt); + + console.log("\n--- Raw result structure ---"); + console.log("Type:", typeof result); + console.log("Keys:", Object.keys(result || {})); + + console.log("\n--- Extracted review ---"); + const reviewText = result?.finalResponse || JSON.stringify(result, null, 2); + console.log(reviewText); + + console.log("\n✓ Test complete"); +} + +test().catch(console.error);