Fix usage example scraper error handling - #804
Conversation
❌ Deploy Preview for splashkit failed.
|
ralphweng2023
left a comment
ralphweng2023
left a comment
There was a problem hiding this comment.
Ran both versions over the current tree. The generated usage-example-references.json comes out byte identical, so this is pure error handling with no output drift. Also reproduced the failure case by leaving a .py with no matching .txt: the old script still says all examples were scraped and exits 0, this one names the missing file and exits 1.
Good catch on the two implicit globals as well. pythonFiles and textFiles were both leaking to global scope. Anchoring the .py to .txt replace is the quiet fix I like most here, the unanchored version would rewrite the wrong part of any path with .py earlier in it. Approved.
ayushsdeakin
left a comment
ayushsdeakin
left a comment
There was a problem hiding this comment.
- Duplicate declarations: A few variables (pythonFiles, pythonFile, textFile, funcEntry, etc.) are declared more than once, which will cause errors. Might be worth cleaning these up.
- Unused variable: textFiles is declared but doesn't seem to be used anywhere. You could probably remove it.
- Filename validation: It might be worth checking if pyFileMatch exists before accessing pyFileMatch[1], otherwise an invalid filename could cause the script to crash.
| const files = fs.readdirSync(folderPath); | ||
| // Filtering for JSON files | ||
| pythonFiles = files.filter(file => path.extname(file).toLowerCase() === '.py'); | ||
| textFiles = files.filter(file => path.extname(file).toLowerCase() === '.txt'); |
There was a problem hiding this comment.
textFiles is created but never used please consder removing it if it isn't required.
There was a problem hiding this comment.
hmm...This line is part of the removed code (red highlight), it's being deleted in this PR. The updated version on the green side no longer declares textFiles.
There was a problem hiding this comment.
yeah, I was reviewing the diff as a whole rather than just the added lines, so I ended up commenting on code that was already on code. running the actual updated code now
| const pythonFile = fs.readFileSync(pythonPath, "utf8"); | ||
| const textFile = fs.readFileSync(textPath, "utf8"); | ||
| const title = textFile.split("\n")[0]; | ||
| const pyFileMatch = fileNameRegex.exec(pyFile); |
There was a problem hiding this comment.
The code uses pyFileMatch[1] without checking whether fileNameRegex.exec(pyFile) actually returned a match. If the filename is invalid,and also this could throw an error. Add validation before accessing the array.
| const pyFileMatch = fileNameRegex.exec(pyFile); | ||
| try { | ||
|
|
||
| try { |
There was a problem hiding this comment.
Nested try/catch blocks make the code hard to follow.
ralphweng2023
left a comment
ralphweng2023
left a comment
There was a problem hiding this comment.
Re-approval at db6f838. The three issues ayushsdeakin raised, namely the duplicate pythonFiles/textFiles declarations across folder iterations, the unused textFiles variable, and the missing filename validation, are all addressed in this diff. pythonFiles is now scoped inside the folder loop, textFiles is gone, and the existsSync check on textPath throws a clear error naming both the missing source and the expected companion. Errors now propagate via throw and process.exitCode = 1 rather than being swallowed, and the array-vs-file log message is fixed. Approved.
rachelpatrao
left a comment
rachelpatrao
left a comment
There was a problem hiding this comment.
Looks good! The scraper now handles missing description files correctly, reports clear errors, and avoids generating incomplete reference data when validation fails. The changes are well tested and everything looks good from my side. Happy to approve!
4 participants
Description
This pull request fixes error handling in the usage-example scraping script.
Previously, when a Python usage example was missing its matching
.txtdescription file, the scraper caught the error too broadly, skipped the affected category, wrote incomplete reference data, printed a success message, and exited with status code0.The updated scraper now:
.txtdescription fileNo new dependencies are required.
Type of change
How Has This Been Tested?
The successful workflow was tested by running:
node scripts/usage-example-scraping.cjsecho "exit=$?"The script completed successfully and returned:
All examples have been scraped successfully.exit=0The normal repository generation workflow was also tested with:
npm run generate-jsonThe failure behaviour was tested by temporarily renaming an existing
.txtdescription file while leaving its matching Python example in place.The scraper correctly:
exit=1usage-example-references.jsonfileThe output JSON was checked before and after the failure using
shasum, and both checksums were identical.Syntax and formatting checks were also run:
node --check scripts/usage-example-scraping.cjsgit diff --checkBoth completed without errors.
Testing Checklist
Checklist
If involving code
Folders and Files Added/Modified
scripts/usage-example-scraping.cjsAdditional Notes
Generated JSON files were restored after testing so this pull request remains focused only on the scraper error-handling fix.
This change improves the reliability of the usage-example generation workflow and gives contributors actionable feedback when an example is incomplete, rather than allowing incomplete reference data to be generated silently.