Conversation
.iced files have never worked, not once, since introduction. And there are no tests to assert that it does. A function that never worked cannot be a breaking change, so this isn't. Addresses node-config#900
📝 WalkthroughWalkthroughThe pull request removes support for iced-coffee-script from the parser module. The ICED_DEP constant, icedParser function, and iced entry from the definitions map and load order are deleted, eliminating the Parser.icedParser public API and its related functionality. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
parser.js (1)
285-304:⚠️ Potential issue | 🔴 CriticalDon't remove the built-in
.icedregistration.Dropping
icedfromorderanddefinitionsmakes.icedfiles unresolvable through the built-in parser path:Parser.parse('config.iced', ...)now falls through toundefined, and callers also losegetParser('iced')/ built-in file-order discovery. That is the opposite of this PR's stated goal of restoring.icedsupport, so the extension needs to stay registered here until the replacement implementation is wired back in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@parser.js` around lines 285 - 304, The change removed the built-in `.iced` registration causing Parser.parse('config.iced', ...) and getParser('iced') to fail; re-add 'iced' into the order array and restore an entry in definitions mapping 'iced' to the built-in parser (e.g., Parser.icedParser) so the extension is discoverable and resolvable via the existing Parser.parse and getParser flows; update both the order variable and the definitions object (referencing the symbols order, definitions, and Parser.icedParser) to restore behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@parser.js`:
- Around line 285-304: The change removed the built-in `.iced` registration
causing Parser.parse('config.iced', ...) and getParser('iced') to fail; re-add
'iced' into the order array and restore an entry in definitions mapping 'iced'
to the built-in parser (e.g., Parser.icedParser) so the extension is
discoverable and resolvable via the existing Parser.parse and getParser flows;
update both the order variable and the definitions object (referencing the
symbols order, definitions, and Parser.icedParser) to restore behavior.
|
addresses #900 |
1 participant
.iced files have never worked, not once, since introduction. And there are no tests to assert that it does. A function that never worked cannot be a breaking change, so this isn't.
Addresses #900
This should ever so slightly improve the speed of scan() and thus library initialization.
Summary by CodeRabbit