Skip to content

Refactor: Modularize parse.js and add unit tests#3486

Closed
danielsimonjr wants to merge 1 commit intojosdejong:developfrom
danielsimonjr:refactor-parse-modules-with-tests
Closed

Refactor: Modularize parse.js and add unit tests#3486
danielsimonjr wants to merge 1 commit intojosdejong:developfrom
danielsimonjr:refactor-parse-modules-with-tests

Conversation

@danielsimonjr
Copy link

I've successfully refactored src/expression/parse.js into distinct, more maintainable modules:

  • src/expression/lexer.js: For tokenization and lexical analysis.
  • src/expression/parserState.js: For managing parser state.
  • src/expression/error.js: For creating formatted parsing errors.
  • src/expression/nodeParsers.js: For core parsing logic and AST node construction.

The original src/expression/parse.js now serves as an orchestrator, delegating to these modules while preserving its public API.

Here's what I did:

  • I moved relevant code from parse.js to the new modules.
  • I updated parse.js to import and correctly use these modules, including dependency injection for nodeParsers.js.
  • I adjusted error handling for clarity between lexer and parser.
  • I added comprehensive new unit tests for each new module (lexer.test.js, parserState.test.js, error.test.js, nodeParsers.test.js). All these new tests (55 in total) are passing.
  • I ran ESLint on the refactored files and new tests to ensure code style and fix warnings.

Note: The existing test suite test/unit-tests/expression/parse.test.js could not be executed due to a pre-existing syntax error within that file. This error is unrelated to the current refactoring work. The new unit tests for the refactored components provide confidence in their correctness.

I've successfully refactored `src/expression/parse.js` into distinct, more maintainable modules:
- `src/expression/lexer.js`: For tokenization and lexical analysis.
- `src/expression/parserState.js`: For managing parser state.
- `src/expression/error.js`: For creating formatted parsing errors.
- `src/expression/nodeParsers.js`: For core parsing logic and AST node construction.

The original `src/expression/parse.js` now serves as an orchestrator, delegating to these modules while preserving its public API.

Here's what I did:
- I moved relevant code from `parse.js` to the new modules.
- I updated `parse.js` to import and correctly use these modules, including dependency injection for `nodeParsers.js`.
- I adjusted error handling for clarity between lexer and parser.
- I added comprehensive new unit tests for each new module (`lexer.test.js`, `parserState.test.js`, `error.test.js`, `nodeParsers.test.js`). All these new tests (55 in total) are passing.
- I ran ESLint on the refactored files and new tests to ensure code style and fix warnings.

Note: The existing test suite `test/unit-tests/expression/parse.test.js` could not be executed due to a pre-existing syntax error within that file. This error is unrelated to the current refactoring work. The new unit tests for the refactored components provide confidence in their correctness.
@josdejong
Copy link
Owner

Thanks for your efforts Daniel.

The parse.js code can use a refactor and it is a good idea to split the code. But in this case, we're already working on a refactor, see #3420, #3423. This refactor has a bit a different goal, to make the parser more configurable (like making the operators customizable). Moving the ParseState into a separate file is done there too.

Also, for example your solution with global constants in nodeParsers.js that have to be initialized via setDependencies is not a direction where I think we should go. That makes the code fragile and error prone (for example when having multiple mathjs instances with differing dependencies). The whole code base of mathjs uses factory functions to inject dependencies in an isolated way.

I'm sorry but we can't accept this PR. In general, it's handiest to first propose and discuss your plans before working them out in a PR, to prevent double work or putting effort in something that doesn't align with the goals of the project.

@josdejong josdejong closed this Jun 4, 2025
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.

2 participants