Repository navigation
Block Python file writes without an extension - #1675
Conversation
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Filename parsing still permits blank component extensions, and the new test does not execute the validation.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds extension validation to Python file writes, complementing the editor’s existing new-file and rename checks.
Changes:
- Rejects extensionless text-file writes with an
OSError. - Adds a worker test checking that the validation message is included.
| File | Description |
|---|---|
src/PyodideWorker.js |
Adds extension validation for text-file writes. |
src/components/Editor/Runners/PythonRunner/PyodideRunner/PyodideWorker.test.js |
Checks for the validation message in injected Python source. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Block names with an empty part between dots (e.g. "test..txt"), which the New File modal already rejects - Split file names in handleFileWrite like NewFileModal does (extension = everything after the first dot), so a write can't create a component with a blank extension - Add Cypress tests for extensionless writes in w/a/x modes and for names with consecutive dots
zetter-rpf
left a comment
zetter-rpf
left a comment
There was a problem hiding this comment.
Nice one on finding the cause for this, and I agree this change should reduce the errors.
What I'm unsure about is if we should require files to have an extension. I can see some value in this restriction when creating files in the editor so that a person doesn't accidentally create a non *.py file that won't run/import, but if the file is being created programatically I'm not sure what the purpose of this restriction is. The volume of errors makes me wonder if there might be project that gets people to write files that might be working.
What do you think?
@zetter-rpf I did try without this, and what I found is that it would create files with a type of undefined. So for example |
I think we would need to change that behaviour too if we wanted to allow files without extensions |
closes https://github.com/RaspberryPiFoundation/digital-editor-issues/issues/1555
This fixes users creating blank extensions by using code like -
Previously we added validation to the new file/rename modals, but this is a way to create files without an extension still.