Conversation
…ce.getConfiguration instead of using our custom config proxy to make the config per-doc
🦋 Changeset detectedLatest commit: 143990f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughParedit configuration moved from a global/static field to per-document runtime reads; package schema and README updated to allow Changes
Sequence Diagram(s)sequenceDiagram
participant Editor as "VSCode Editor (Document)"
participant Command as "Paredit Command Handler"
participant Config as "VSCode Config (emacs-mcx)"
participant Paredit as "paredit.reader"
participant Logger as "Logger"
Editor->>Command: invoke paredit action (with Document)
Command->>Config: getConfiguration("emacs-mcx", Document) -> get("paredit.parentheses")
Config-->>Command: returns mapping (may include nulls)
Command->>Logger: log active parentheses mapping
Command->>Paredit: setParentheses(filteredMapping)
Command->>Paredit: perform sexp-travel / edits
Paredit-->>Editor: apply edits
Paredit->>Logger: emit operation logs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where the emacs-mcx.paredit.parentheses configuration option was defined in the schema but never actually read and applied. The PR refactors the paredit configuration to be effective per document rather than globally, enabling language-specific parentheses configuration.
Changes:
- Removed global paredit configuration from the Configuration class
- Added per-document configuration retrieval in paredit commands
- Enhanced package.json schema to support
nullvalues for disabling specific pairs and addedscope: "language-overridable" - Updated documentation to describe the new configuration behavior
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/configuration/iconfiguration.ts | Removed IPareditConfiguration interface and paredit property from IConfiguration interface |
| src/configuration/configuration.ts | Removed paredit configuration property and related imports, eliminating global paredit.js setup |
| src/commands/paredit.ts | Added getPareditParenthesesConfig() function to retrieve per-document configuration, integrated it into makeSexpTravelFunc(), added debug logging, and improved code style with replaceAll() |
| package.json | Modified schema to allow null values for parentheses pairs and added scope: "language-overridable" for per-language configuration |
| README.md | Updated documentation with examples showing how to add new pairs and disable default pairs |
| .changeset/slick-owls-watch.md | Added changeset describing the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/commands/paredit.ts`:
- Around line 42-45: The code is calling an undocumented internal API
paredit.reader.setParentheses from the getPareditParenthesesConfig flow; locate
the call site around getPareditParenthesesConfig and
paredit.reader.setParentheses and either (a) confirm and use a local wrapper
(e.g., implement and export a stable setParentheses function in
vendor/paredit.js and call that), (b) confirm the method is part of paredit's
documented public API and replace the current access with the documented
accessor, or (c) refactor to configure parentheses via paredit's supported
public API (e.g., initialization/options or a documented setter) instead of
touching paredit.reader directly; ensure logger.debug still logs the resolved
parentheses and remove direct references to paredit.reader.setParentheses unless
the API is proven stable or wrapped locally.
In `@src/configuration/configuration.ts`:
- Line 7: The import line referencing the module name "iconfiguration" triggers
a cspell failure; update the import comment or dictionary so CI passes by either
adding a cspell ignore comment above the import or adding "iconfiguration" to
the project's cspell dictionary. Locate the import statement that brings in
IConfiguration and IDebugConfiguration (the line importing from
"./iconfiguration") and either prepend a cspell:disable/enable comment for that
word or add "iconfiguration" to cspell.json/words to whitelist it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/test/suite/extension.native.test.ts`:
- Around line 70-73: Update the inline comment that currently reads it only
handles "subwordMode" to also mention "paredit" and explain the different
handling: note that "subwordMode" is handled by wordSeparators.ts while
"paredit" is excluded because it is applied per-document at runtime in the
paredit command handler; update the comment adjacent to the if statement
checking keyFirstSegment and the array ["subwordMode", "paredit"] to reflect
both behaviours and their handlers.
VSIX File Tree |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const parentheses = getPareditParenthesesConfig(doc); | ||
| logger.debug(`Using paredit parentheses: ${JSON.stringify(parentheses)}`); | ||
| paredit.reader.setParentheses(parentheses); |
There was a problem hiding this comment.
The call to paredit.reader.setParentheses modifies global state in the paredit.js library. If multiple paredit commands are executed concurrently on different documents with different configurations, they could interfere with each other. For example, if command A sets parentheses for document A, then command B sets different parentheses for document B before command A calls paredit.parse, command A will use the wrong configuration.
While this is likely a pre-existing architectural limitation of paredit.js rather than a new issue introduced by this PR, it's worth noting. A potential mitigation would be to ensure paredit operations are serialized, though this may be complex to implement correctly.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Looks like the web test fails due to the error on the VSCode upstream that seems to be fixed by microsoft/vscode#292948 |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores