X Tutup
Skip to content

Convert parser.js to a class#898

Open
jdmarshall wants to merge 1 commit intonode-config:masterfrom
jdmarshall:parser
Open

Convert parser.js to a class#898
jdmarshall wants to merge 1 commit intonode-config:masterfrom
jdmarshall:parser

Conversation

@jdmarshall
Copy link
Collaborator

@jdmarshall jdmarshall commented Mar 5, 2026

Summary by CodeRabbit

  • Refactor
    • Improved parser architecture for better extensibility and API consistency. The parser now supports custom parser registration and retrieval through updated public methods, enabling more flexible file format handling.

@jdmarshall jdmarshall added this to the 5.0 milestone Mar 5, 2026
@coderabbitai
Copy link

coderabbitai bot commented Mar 5, 2026

📝 Walkthrough

Walkthrough

The parser.js module is refactored from a namespace-like object pattern with static-like functions to a proper class-based architecture. Individual parser functions become instance methods, definitions and order become prototype properties, and exports change to provide both a default singleton instance and the Parser class itself.

Changes

Cohort / File(s) Summary
Parser Class Refactoring
parser.js
Converts parser module from namespace pattern to class-based design. All parser functions (xmlParser, jsParser, coffeeParser, tsParser, yamlParser, jsonParser, json5Parser, hjsonParser, tomlParser, csonParser, propertiesParser) become instance methods. Reorganizes definitions map and order array as prototype properties. Updates getParser/setParser/getFilesOrder/setFilesOrder to instance methods. Exports default instance (new Parser()) plus Parser class. Reworks dependency initialization and adds utility methods (stripYamlComments, booleanParser, numberParser).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Poem

A rabbit hops with joy so keen,
Classes now, where objects had been!
Instance methods, bright and new,
Parser's structure fresh and true. 🐰

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately describes the main change: converting parser.js from a namespace-like object to a class-based architecture with instance methods.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@parser.js`:
- Around line 54-58: The Parser's shared module-level registries (definitions
and order) cause cross-instance leakage; make parser-specific registries by
moving definitions and order into the Parser class (e.g., as this.definitions
and this.order) and update all methods that reference them—parse, setParser,
setFilesOrder and any logic between lines ~320-391—to use instance properties
instead of the module globals so each new Parser() gets its own isolated
registry and files order.
- Around line 160-167: icedParser currently registers the Iced loader (via
Iced.register()) but never loads or returns the parsed module, causing callers
to get undefined; modify icedParser so after calling Iced.register() it actually
loads the target file (using the provided filename/content - e.g.,
require(filename) or compile the provided content into the module) and returns
the module's exported config object; ensure the returned value is the parsed
config and keep existing uses of Iced.register and ICED_DEP intact.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f21c275b-a044-42d4-969f-7042f8528d2f

📥 Commits

Reviewing files that changed from the base of the PR and between b0c9c9d and 28236bf.

📒 Files selected for processing (1)
  • parser.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

X Tutup